Closed Bug 1604143 Opened 4 months ago Closed 3 months ago

Make browser-chrome task function arguments an object

Categories

(Remote Protocol :: Agent, enhancement, P3)

enhancement

Tracking

(firefox74 fixed)

RESOLVED FIXED
Firefox 74
Tracking Status
firefox74 --- fixed

People

(Reporter: ato, Assigned: kstheking0, Mentored)

References

Details

(Whiteboard: [lang=js])

Attachments

(1 file, 3 obsolete files)

Task functions in browser-chrome tests currently take a pre-defined list of arguments:

add_task(async function testName(client, CDP, tab) {
  …
});

whimboo suggested in D57078 that an object would be better because it allows the consumer to pick which arguments she needs, and allows us to more easily introduce additional fixtures for the tests.

Depends on bug 1603451 because it introduces documentation that must be updated.

Depends on: 1603451
Priority: -- → P3
Whiteboard: [puppeteer-beta-reserve]

My proposal would be:

add_task(async function testName({ client, CDP, tab } = {}) {
  const { Page, Runtime } = client;
  …
});

To make that change the code in https://searchfox.org/mozilla-central/source/remote/test/browser/head.js needs to be updated, so it passes an object into the test method.

Also all browser chrome tests under https://searchfox.org/mozilla-central/source/remote/test/browser/ would have to be fixed to make use of the above proposed code.

I see this as a great mentored bug but not something which should block our beta MVP.

To get started please check our documentation at https://firefox-source-docs.mozilla.org/remote/.

Mentor: hskupin
Whiteboard: [puppeteer-beta-reserve] → [lang=js]

Hello, I'd like to work on this bug. If allowed, as I'm a beginner in contributing to mozilla code base, I will be really grateful for guidance on how do I go about doing this

Flags: needinfo?(hskupin)

You are welcome to work on this bug. As noted in my last comment please read through the docs and ask questions if something is unclear or not listed. All the information you need is basically there.

Flags: needinfo?(hskupin)

Hello, Sorry for commenting so late, I was travelling
I tried changing as described above i.e. I changed the add_task function in head.js so that it passes an object {client, CDP, tab} into the test function
and I also changed files such as browser_cdp.js so that it accepts an object
Now after running tests they start failing at tests inside 'emulation' with errors like
TypeError: Emulation is undefined
which is probably because I am not passing anything other than client, CDP and tab in the task function

Can I get an idea of how do I go about solving this and if I am misinterpreting something

Flags: needinfo?(hskupin)

It would help if you could share the code so that I can check which parts you changed. But I believe you didn't destruct Emulation from client. It should look like:

add_task(async function raisesWithUnknownTargetId({ client, tab }) {
  const {Emulation, Target } = client;
  [..] 
}
Flags: needinfo?(hskupin)

hi, I did as above and all the tests have passed
Should I run moz-phab and submit a patch for code review, I don't know the exact details if something needs to be done first
Also, I am using git cinnabar for working with gecko, does it make a difference and should I be using hg directly ?

Flags: needinfo?(hskupin)

You can use moz-phab or phlay to submit your patches. Make sure the commit message is well-formatted. It doesn’t matter if you use git or hg. I personally use git.

Flags: needinfo?(hskupin)
Assignee: nobody → kstheking0
Status: NEW → ASSIGNED

I have submitted the patch, please review it

Flags: needinfo?(hskupin)

Note that there is no need for a needinfo. Phabricator automatically informs me when there are reviews. But thanks for uploading it. I or someone else from our team will have a look at it today.

Flags: needinfo?(hskupin)

Oh ok I didn't know that, Thanks

Depends on D59082

Depends on D59255

Depends on D59256

Attachment #9119619 - Attachment is obsolete: true
Attachment #9119618 - Attachment is obsolete: true
Attachment #9119617 - Attachment is obsolete: true

Note to all: This commit contains a sizable refactoring of browser-chrome tests under remote/test/browser/**/*.js. If you have work-in-progress patches that make changes you may have to rebase your work.

Hello, I have pulled the default branch and fixed the conflicts, rebased the changes into the previous commit and I can update these changes to the patch
however when I run git rebase -i central'
I get error
fatal: Needed a single revision invalid upstream 'central'
Should I still run git rebase ? and if I should then what should I do to fix the error

Flags: needinfo?(hskupin)

Are you using git or hg? I assumed hg. If it's git Andreas would have to help.

Flags: needinfo?(hskupin)

I am using git

Flags: needinfo?(ato)

I suspect you haven’t set up central to track the upstream git server’s bookmarks/central branch.

Try maybe the following:

% git rebase -i bookmarks/central

Hopefully your default origin will be the upstream remote, otherwise you need to also append the remote’s name.

You can list your remotes with git remote -v.

The relevant subset of my .git/config is:

[remote "mozilla"]
	url = hg::https://hg.mozilla.org/mozilla-unified
	fetch = +refs/heads/bookmarks/*:refs/remotes/mozilla/*
	pushurl = ssh://mime.sny.no/git/gecko.git
	pushurl = git@github.com:andreastt/gecko.git

[branch "central"]
	remote = mozilla
	merge = refs/heads/bookmarks/central
	rebase = true
Flags: needinfo?(ato)

I ran git rebase -i origin/bookmarks/central
and after fixing the conflicts I had about 31 commits ahead of me, certainly running moz-phab would create seperate 31 phabricator instances so I ran git rebase again and squashed them all in one commit and updated the patch
But from the looks of the patch I think I have done something wrong , as it says I modified about 197 files
What should I do ?

Flags: needinfo?(ato)

Should I manually revert those files (the ones which I wasn't supposed to change) by comparing with the central bookmark

I fixed the above mistake, the patch should be fine now

Flags: needinfo?(ato)
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ef16eb363c08
Make browser-chrome task function arguments an object. r=whimboo,remote-protocol-reviewers,ato

Thanks again for the patch, Khushal!

Yes, Thank you for your help and patience !

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 74
You need to log in before you can comment on or make changes to this bug.