1065446 - Simplify and document running browser_parsable_script.js in debug builds

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Paolo Amadini]

browser_parsable_script.js is disabled in debug builds because it takes a long time, but it was not clear that it works correctly (rather than blocking forever).

This bug adds a way to run the test in debug builds, in case a failure is observed in an optimized build.

Comment 1

[:Paolo Amadini]

Attachment: The patch

Comment 2

[:Gijs (he/him)]

Comment on attachment 8487266 [details] [diff] [review]
The patch

Review of attachment 8487266 [details] [diff] [review]:
-----------------------------------------------------------------

f+, but I can upgrade to r+ if you don't want to use a full path, I guess?

This should also have a try push before landing. I'm pretty scared of this change after the bumpy landing the original patch had with debug/asan failures.

::: browser/base/content/test/general/browser_parsable_script.js
@@ +62,5 @@
>  add_task(function* checkAllTheJS() {
> +  // In debug builds, even on a fast machine, collecting the file list may take
> +  // more than 30 seconds, and parsing all files may take four more minutes.
> +  // For this reason, this test must be explictly requested in debug builds by
> +  // using the "--setpref parse=<filter>" argument to mach.  You can specify a

Are we sure we're comfortable with such a generic pref?

@@ +72,5 @@
> +    if (!parseRequested) {
> +      info("Test disabled on debug build. To run, execute:" +
> +           " ./mach mochitest-browser --setpref parse=<case_sensitive_filter>" +
> +           " browser/base/content/test/general/browser_parsable_script.js");
> +      return;

You should add a trivial ok(true, "Msg") instead of the info() because otherwise this will cause a test failure because the test does not test anything.

@@ +91,5 @@
>    // We create an array of promises so we can parallelize all our parsing
>    // and file loading activity:
>    let allPromises = [];
>    for (let uri of uris) {
> +    if (parseFilter && uri.spec.indexOf(parseFilter) == -1) {

Nit: .contains(parseFilter)


It kind of bothers me that this will still spend 30 seconds or more looking through all the files. Can we not ask that people specify the full path to the file as an argument, and then not bother doing a generateURIsFromDirTree?

Comment 3

[:Paolo Amadini]

(In reply to :Gijs Kruitbosch from comment #2)
> Are we sure we're comfortable with such a generic pref?

I am, this is only used to pass a value to this specific test when using "mach", no need for more complicated names.

> It kind of bothers me that this will still spend 30 seconds or more looking
> through all the files. Can we not ask that people specify the full path to
> the file as an argument, and then not bother doing a generateURIsFromDirTree?

The URI looks like:

file:///Users/paolo/moz/obj-x86_64-apple-darwin/dist/NightlyDebug.app/Contents/MacOS/browser/modules/loop/MozLoopAPI.jsm

Not really portable when you want to give someone else a command line to run the same test.

But I can add a check for absolute URIs (contains ":") that shortcuts the collection phase.

Comment 4

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #3)
> (In reply to :Gijs Kruitbosch from comment #2)
> > Are we sure we're comfortable with such a generic pref?
> 
> I am, this is only used to pass a value to this specific test when using
> "mach", no need for more complicated names.
> 
> > It kind of bothers me that this will still spend 30 seconds or more looking
> > through all the files. Can we not ask that people specify the full path to
> > the file as an argument, and then not bother doing a generateURIsFromDirTree?
> 
> The URI looks like:
> 
> file:///Users/paolo/moz/obj-x86_64-apple-darwin/dist/NightlyDebug.app/
> Contents/MacOS/browser/modules/loop/MozLoopAPI.jsm
> 
> Not really portable when you want to give someone else a command line to run
> the same test.
> 
> But I can add a check for absolute URIs (contains ":") that shortcuts the
> collection phase.

mm, the relative path (browser/modules/loop/MozLoopAPI.jsm) is probably OK, but that's going to be annoying to figure out for the user, I guess. :-(

Let's take this patch (with the other comments addressed) for now and then we can iterate if necessary.

Comment 5

[:Paolo Amadini]

Attachment: Updated patch

Already made the requested change...

https://tbpl.mozilla.org/?tree=Try&rev=7ed34a35238f

Comment 6

[:Paolo Amadini]

This run should detect the error in the optimized build only:

https://tbpl.mozilla.org/?tree=Try&rev=efaeae5c7eab

Comment 7

[:Paolo Amadini]

Fixed and pushed to fx-team:

https://hg.mozilla.org/integration/fx-team/rev/3f3e0c97c988

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3f3e0c97c988