Closed
Bug 1065446
Opened 10 years ago
Closed 10 years ago
Simplify and document running browser_parsable_script.js in debug builds
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 35
People
(Reporter: Paolo, Assigned: Paolo)
References
Details
Attachments
(1 file, 1 obsolete file)
4.70 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
Attachment #8487266 -
Flags: review?(gijskruitbosch+bugs)
Comment 2•10 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•10 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•10 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•10 years ago
|
||
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•10 years ago
|
Attachment #8487299 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 6•10 years ago
|
||
This run should detect the error in the optimized build only:
https://tbpl.mozilla.org/?tree=Try&rev=efaeae5c7eab
Assignee | ||
Comment 7•10 years ago
|
||
Fixed and pushed to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/3f3e0c97c988
Comment 8•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in
before you can comment on or make changes to this bug.
Description
•