1604143 - Make browser-chrome task function arguments an object

Status: RESOLVED FIXED

(Remote Protocol :: Agent, enhancement, P3)

Description

[Andreas Tolfsen ❲:ato❳]

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.

Comment 1

[Andreas Tolfsen ❲:ato❳]

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

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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/.

Comment 3

[Khushal Sahni]

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

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 5

[Khushal Sahni]

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

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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;
  [..] 
}

Comment 7

[Khushal Sahni]

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 ?

Comment 8

[Andreas Tolfsen ❲:ato❳]

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.

Comment 9

[Khushal Sahni]

Attachment: Bug 1604143 - Make browser-chrome task function arguments an object. r=whimboo

Comment 10

[Khushal Sahni]

I have submitted the patch, please review it

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 12

[Khushal Sahni]

Oh ok I didn't know that, Thanks

Comment 13

[Khushal Sahni]

Attachment: Bug 1604143 - Make browser-chrome task function arguments an object. r=whimboo

Depends on D59082

Comment 14

[Khushal Sahni]

Attachment: Bug 1604143 - Make browser-chrome task function arguments an object. r=whimboo

Depends on D59255

Comment 15

[Khushal Sahni]

Attachment: Bug 1604143 - Make browser-chrome task function arguments an object. r=whimboo

Depends on D59256

Comment 16

[Andreas Tolfsen ❲:ato❳]

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.

Comment 17

[Khushal Sahni]

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

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Comment 19

[Khushal Sahni]

I am using git

Comment 20

[Andreas Tolfsen ❲: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 = [e-mail]:andreastt/gecko.git

[branch "central"]
	remote = mozilla
	merge = refs/heads/bookmarks/central
	rebase = true

Comment 21

[Khushal Sahni]

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 ?

Comment 22

[Khushal Sahni]

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

Comment 23

[Khushal Sahni]

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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef16eb363c08
Make browser-chrome task function arguments an object. r=whimboo,remote-protocol-reviewers,ato

Comment 25

[Andreas Tolfsen ❲:ato❳]

Thanks again for the patch, Khushal!

Comment 26

[Khushal Sahni]

Yes, Thank you for your help and patience !

Comment 27

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/ef16eb363c08