Closed Bug 1593979 Opened 7 months ago Closed 5 months ago

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

Categories

(Remote Protocol :: Agent, defect, P1)

defect

Tracking

(firefox71 unaffected, firefox72 wontfix, firefox73 fixed)

RESOLVED FIXED
Firefox 73
Tracking Status
firefox71 --- unaffected
firefox72 --- wontfix
firefox73 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [puppeteer-beta-mvp])

Attachments

(1 file)

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?

Flags: needinfo?(gbrown)

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.

Flags: needinfo?(gbrown)

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

Mentor: hskupin
Whiteboard: [lang=js]

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

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

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

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

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?

Flags: needinfo?(hskupin)

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

however, input test also runs but has failures.

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.

Flags: needinfo?(hskupin)

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.

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

Assignee: nobody → walshd9

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

Flags: needinfo?(cmanchester)
Flags: needinfo?(ahal)

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/

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?

Flags: needinfo?(hskupin)

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.

Flags: needinfo?(hskupin)

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.

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.
Flags: needinfo?(ahal)

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.

Flags: needinfo?(cmanchester)

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

Flags: needinfo?(hskupin)

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.

Flags: needinfo?(hskupin) → needinfo?(walshd9)

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.

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.

Assignee: walshd9 → nobody
Flags: needinfo?(walshd9)

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

Assignee: nobody → hskupin
Mentor: hskupin
Status: NEW → ASSIGNED
Priority: P3 → P1
Attachment #9111312 - Attachment description: Bug 1593979 - mach test for remote/test/browser/*/ fails after a clobber with head.js not found, r=whimboo → Bug 1593979 - [remote] Always include global head.js and chrome-remote-interface.js.

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

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

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.

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 73
Whiteboard: [lang=js] → [puppeteer-beta-mvp]
You need to log in before you can comment on or make changes to this bug.