Simplify and document running browser_parsable_script.js in debug builds

RESOLVED FIXED in Firefox 35

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

Trunk
Firefox 35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8487266 [details] [diff] [review]
The patch
Attachment #8487266 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

4 years ago
Blocks: 1065450

Comment 2

4 years ago
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+
(Assignee)

Comment 3

4 years ago
(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

4 years ago
(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.
(Assignee)

Comment 5

4 years ago
Created attachment 8487299 [details] [diff] [review]
Updated patch

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)

Updated

4 years ago
Attachment #8487299 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 6

4 years ago
This run should detect the error in the optimized build only:

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

Comment 7

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.