Closed Bug 1065446 Opened 6 years ago Closed 6 years ago

Simplify and document running browser_parsable_script.js in debug builds

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch The patch (obsolete) — Splinter Review
Attachment #8487266 - Flags: review?(gijskruitbosch+bugs)
Blocks: 1065450
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?
Attachment #8487266 - Flags: review?(gijskruitbosch+bugs) → review+
(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.
(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.
Attached patch Updated patchSplinter Review
Already made the requested change...

https://tbpl.mozilla.org/?tree=Try&rev=7ed34a35238f
Attachment #8487266 - Attachment is obsolete: true
Attachment #8487299 - Flags: review?(gijskruitbosch+bugs)
Attachment #8487299 - Flags: review?(gijskruitbosch+bugs) → review+
This run should detect the error in the optimized build only:

https://tbpl.mozilla.org/?tree=Try&rev=efaeae5c7eab
Fixed and pushed to fx-team:

https://hg.mozilla.org/integration/fx-team/rev/3f3e0c97c988
https://hg.mozilla.org/mozilla-central/rev/3f3e0c97c988
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.