1593979 - "mach test" for remote/test/browser/*/ fails after a clobber with head.js not found

Status: RESOLVED FIXED

(Remote Protocol :: Agent, defect, P1)

Description

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

If you build for the first time or after a clobber and directly run the eg. following command, the test will fail:

./mach test remote/test/browser/target/

0:06.15 GECKO(92232) Chrome file doesn't exist: /Volumes/code/gecko/obj/debug/_tests/testing/mochitest/browser/remote/test/browser/head.js
 0:06.15 FAIL head.js import threw an exception - Error opening input stream (invalid filename?): chrome://mochitests/content/browser/remote/test/browser/head.js

Reason here is that only head.js from within the target directory is packaged as support file, but not the one from the parent directory. Given that we now have all of our browser chrome tests divided into sub dirs (bug 1589844, bug 1591006), there is no back-reference to the top-level head.js.

Also some of those folders have their own custom code in their own head.js file, like here for Input:

https://searchfox.org/mozilla-central/source/remote/test/browser/input/head.js

Geoff, do you have a suggestion for us in how to have custom head.js files for subfolders, but also to always package the global one which is one level higher? I mean we could have something like the following for the browser.ini file in sub folders:

support-files =
    head.js
    ../head.js

But maybe there is a more elegant solution?

Comment 1

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

A data point: the only other bc tests organized into subdirs are at accessible/tests/browser; they list the parent head.js in the child browser.ini.

Comment 2

[Geoff Brown [:gbrown]]

I found a similar situation in

https://searchfox.org/mozilla-central/rev/3300072e993ae05d50d5c63d815260367eaf9179/browser/tools/mozscreenshots/controlCenter/browser.ini#3-4

I think

support-files =
    head.js
    ../head.js

is the way to go.

Comment 3

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

So we should actually do the same. Thanks a lot. Given that it is not ultra-critical I will set it up as mentored bug.

To get this fixed have a look at each of the browser.ini files inside the sub folders of:
https://searchfox.org/mozilla-central/source/remote/test/browser/

Make the change as requested above, and run the tests in each folder like:

./mach test remote/test/browser/input/

To fully test you will have to do a mach clobber and mach build before. Therefor it would be best to make use of an artifact build (run ./mach bootstrap with option 1).

Comment 4

[Dan Walsh]

Hi, I'm looking to try a second bug and would like to take this on if it is still available?

Comment 5

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

Sure, feel free to pick it up. Thanks a lot!

Comment 6

[Dan Walsh]

So from reading above, each subfolder from "browser" needs to have "../head.js" added to its browser.ini file then test each subfolder. Is this correct?
Also, I was wondering if there's a page that briefly explains how to use markup in comments to highlight code, etc?
Cheers

Comment 7

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

(In reply to Dan Walsh from comment #6)

So from reading above, each subfolder from "browser" needs to have "../head.js" added to its browser.ini file then test each subfolder. Is this correct?

Yes that is true. You can test that changes work when doing a clobber, a build and directly try to run tests in that particular sub folder which you have changed. If it passes the patch is correct.

Also, I was wondering if there's a page that briefly explains how to use markup in comments to highlight code, etc?

That is Bugzilla related. But it's all Markdown syntax, similar to what you would use on Github. For code blocks see https://www.markdownguide.org/extended-syntax/#fenced-code-blocks.

Comment 8

[Dan Walsh]

Hi, I've made the changes to the file then built and then ran the test:
./mach test remote/test/browser/input/

It now runs but fails with;
ValueError: Item already in manifest: testing/mochitest/browser/remote/test/browser/target/head.js

It then lists nine files with a line number and function name for each.

Do I need to do something with this or commit the fix, that is related to this bug then these failures come under a different bug?

Comment 9

[Dan Walsh]

I meant to say that I had run the test:
./mach test remote/test/browser/target

however, input test also runs but has failures.

Comment 10

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

Hm, I assume this happens because we have custom head.js files in some of the sub folders. When I take runtime for example, it works just fine.

Maybe the manifest parser code extracts the base filename, and then recognizes that head.js has already been added? You could have a look at various methods as listed in the printed stack, while going from the bottom to the top.

Comment 11

[Dan Walsh]

I'm not sure, because input test runs fine without a value error, but fails due to an "uncaught exception" during mochitests ("Error opening input stream").
I'll try running all the other tests and see if the value error occurs in any of the others.

Comment 12

[Dan Walsh]

Hi, I've found that using:
../head.js
is causing the issue and that instead, we should probably use:
!/remote/test/browser/head.js

Comment 13

[Dan Walsh]

Attachment: Bug 1593979 - [remote] Always include global head.js and chrome-remote-interface.js.

Comment 14

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

(In reply to Dan Walsh from comment #11)

I'm not sure, because input test runs fine without a value error, but fails due to an "uncaught exception" during mochitests ("Error opening input stream").

No, the input tests don't run either and everything is marked as fail. This is because the chrome-remote-interface.js file cannot be found, which gets packaged because of the supported files entries in the remote/test/browser/browser.ini file but ends-up under e.g. test/browser/page.

So I'm basically out of ideas what is the right solution here. Maybe Chris or Andrew can help us. So here a short summary:

• We have browser chrome tests under remote/test/browser, which are separated into sub-folders
• All of those need the global head.js, and chrome-remote-interface.js from inside the top folder
• Running e.g. mach test remote/test/browser/page doesn't package those global files
• Adding references to those files in eg. page/browser.ini will instead duplicated those into each of the subfolders, whereby it fails for head.js if it is existent in both folders

I thought that with this patch the ancestor of a manifest will automatically be parsed and evaluated. But as it looks like that isn't done.

Comment 15

[Dan Walsh]

I've run the following commands:

./mach test remote/test/browser/
./mach test remote/test/browser/
./mach test remote/test/browser/
./mach test remote/test/browser/
./mach test remote/test/browser/

Comment 16

[Dan Walsh]

Sorry about the above comment, it was meant to say:
I've run the following commands (with the edits made in my commit "D54568"):

./mach test remote/test/browser/
./mach test remote/test/browser/emulation/
./mach test remote/test/browser/input/
./mach test remote/test/browser/network/
./mach test remote/test/browser/page/
./mach test remote/test/browser/runtime/
./mach test remote/test/browser/security/
./mach test remote/test/browser/target/

all of which have completed with 0 failures.
What tests are you running to produce the errors?

Comment 17

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

Did you ensure to run ./mach clobber && ./mach build before? Once you run ./mach test remote/test/browser/ it will all be setup, and you don't see this problem.

Comment 18

[Dan Walsh]

Yes, I ran;
./mach clobber
./mach build

before running the tests I listed above.
As I said in an earlier comment, I found the issue with the missing files to be caused by the use of:
../head.js

and instead I used:
!/remote/test/browser/head.js

This is in the commit I submitted.

Comment 19

[Andrew Halberstadt [:ahal]]

Sorry I'm having trouble parsing the question there. Could you rephrase?

Couple misc points that may help:

1. I'm fairly sure that the DEFAULTS of ancestor manifests are applied, otherwise a lot of things would be failing.
2. I vaguely remember an ! syntax for support-files that was used when things needed to be shared.. I don't remember when or how it was supposed to be used though.

Comment 20

[Chris Manchester (limited bugmail, email directly)]

I believe the !/ syntax is correct here. It's documented briefly here: https://firefox-source-docs.mozilla.org/build/buildsystem/test_manifests.html#recognized-metadata

../head.js should probably work too, I'm seeing that in a few places in the tree (e.g. browser/tools/mozscreenshots/controlCenter/browser.ini), if we're rejecting that because of special handling of head.js that makes us think there's a duplicate file that might be a bug.

Comment 21

[Dan Walsh]

Does this mean that my commit is ok?
As it runs without errors or failures.

Comment 22

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

Dan, sorry for the delay with my reply. But here come some good news!

Given the feedback from Chris and some further testing the following entries in sub manifest files actually make it work for me:

support-files =
  head.js
  !/remote/test/browser/chrome-remote-interface.js
  !/remote/test/browser/head.js

As you can see your patch misses to add the reference to the chrome-remote-interface.js file to each of the sub manifest files. Can you please correct that in your patch? Thanks.

Comment 23

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

Dan, if you are not able to finish your patch please let us know. Otherwise I will open up this bug for someone else to finish it off. Thanks.

Comment 24

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

Resetting the assignee for now. Dan feel free to pick it up again if you still have interest to get this fixed. Otherwise someone else might pick it up soon. Thanks.

Comment 25

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

It's an annoying bug, so I will pick this up myself now.

Comment 26

[Andreas Tolfsen ❲:ato❳]

Relying on an explicit ordering in support-files seems fragile, but I can’t think of a better way.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cb40d5356066
[remote] Always include global head.js and chrome-remote-interface.js. r=remote-protocol-reviewers,ato

Comment 28

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

No, we do not have to rely on the order. It's just that we have to include the upper-level support files anywhere in that list.

Comment 29

[Alexandru Michis [:malexandru]]

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