Closed
Bug 1280362
Opened 8 years ago
Closed 6 years ago
Clean up jstests' shell.js and browser.js to do everything in IIFEs, with explicit functionality exports for exposure to tests
Categories
(Core :: JavaScript Engine, defect, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: Waldo, Assigned: Waldo)
References
Details
(Keywords: triage-deferred)
Attachments
(24 files)
The "API" exposed to jstests is a bit vague -- a bunch of function names that probably nobody ever fully uses. And exactly what things tests cannot do, because doing so would break the harness that runs after the test code has, is wholly undocumented. We should make shell.js/browser.js do all their work in IIFEs, and when they want to add a function to the global object for use in tests, they should do it via an exports.assertEq = ...-style process that makes those exports extremely clear. I have a dozen patches that *start* work on adding IIFEs, making the relied-upon functionality in exported functions not depend upon test-modifiable semantics (by caching primordial functionality that can't be affected by the test's subsequent shenanigans), &c. There's still a ton more to do on this point even past those patches, so this'll probably remain open for a bit as we slog through it all. Preliminarily, I should note that these dozen-plus patches I'm uploading do *not* work in some small detail, but I don't know which detail. This is mostly to get a ball rolling, and if it turns out the relevant buglet is complex somehow, I'll repost the relevant part for rereview.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8763007 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 2•8 years ago
|
||
Fair warning, these changes are probably laboriously minimal. :-) Better than mega-patches that are unreviewable.
Attachment #8763008 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8763009 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8763010 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 5•8 years ago
|
||
The one-liner used to perform these changes (in js/src/tests) was: find . -name '*.js' | xargs perl -ni -e 'print unless /^\s*jit'\\'(''(true|false)'\\');\s*$/'
Attachment #8763011 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8763012 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8763013 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #8763014 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 9•8 years ago
|
||
Attachment #8763015 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8763017 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8763018 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 12•8 years ago
|
||
Attachment #8763019 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8763020 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 14•8 years ago
|
||
Attachment #8763021 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 15•8 years ago
|
||
Attachment #8763022 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 16•8 years ago
|
||
And, immediate patchbombing complete. :-)
Comment 17•8 years ago
|
||
Comment on attachment 8763007 [details] [diff] [review] Begin to make jstests' shell.js/browser.js less dependent on tests being well-behaved, and make exporting functionality a more deliberate choice Review of attachment 8763007 [details] [diff] [review]: ----------------------------------------------------------------- Thanks :) Will review remaining parts gradually, hopefully within this week. ::: js/src/tests/shell.js @@ +34,5 @@ > + var assertEq = global.assertEq; > + if (typeof assertEq !== "function") { > + assertEq = function assertEq(actual, expected, message) { > + if (!SameValue(actual, expected)) > + { would be better moving this "{" to the end of the previous line
Attachment #8763007 -
Flags: review?(arai.unmht) → review+
Updated•8 years ago
|
Attachment #8763008 -
Flags: review?(arai.unmht) → review+
Comment 19•8 years ago
|
||
Comment on attachment 8763009 [details] [diff] [review] Move DocumentWrite into browser.js's IIFE Review of attachment 8763009 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/tests/browser.js @@ +38,5 @@ > + function DocumentWrite(s) { > + try { > + var msgDiv = global.document.createElement('div'); > + msgDiv.innerHTML = s; > + document.body.appendChild(msgDiv); global.document.body.appendChild
Attachment #8763009 -
Flags: review?(arai.unmht) → review+
Updated•8 years ago
|
Attachment #8763010 -
Flags: review?(arai.unmht) → review+
Updated•8 years ago
|
Attachment #8763011 -
Flags: review?(arai.unmht) → review+
Comment 20•8 years ago
|
||
Comment on attachment 8763012 [details] [diff] [review] Manually remove the remaining jit() call oddballs Review of attachment 8763012 [details] [diff] [review]: ----------------------------------------------------------------- r+ assuming the function in bug646001.js is kept ::: js/src/jit-test/tests/jaeger/bug646001.js @@ -1,3 @@ > -function jit(on) > -{ > - if (on && !options().match(/tracejit/)) { } according to bug 646001 comment #0, this line was necessary to reproduce the issue. not sure if it makes sense to keep whole this testcase tho, at least it would make sense to keep this function here (as renamed?) as long as keeping whole testcase
Attachment #8763012 -
Flags: review?(arai.unmht) → review+
Updated•8 years ago
|
Attachment #8763013 -
Flags: review?(arai.unmht) → review+
Comment 21•8 years ago
|
||
Comment on attachment 8763014 [details] [diff] [review] Move callstack-recording functionality into shell.js's IIFE Review of attachment 8763014 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/tests/shell.js @@ +36,5 @@ > + > + var v = arr[len - 1]; > + arr.length--; > + return v; > + } Any reason not to use Array.prototype.pop and push?
Updated•8 years ago
|
Attachment #8763015 -
Flags: review?(arai.unmht) → review+
Updated•8 years ago
|
Attachment #8763017 -
Flags: review?(arai.unmht) → review+
Updated•8 years ago
|
Attachment #8763018 -
Flags: review?(arai.unmht) → review+
Updated•8 years ago
|
Attachment #8763019 -
Flags: review?(arai.unmht) → review+
Updated•8 years ago
|
Attachment #8763020 -
Flags: review?(arai.unmht) → review+
Updated•8 years ago
|
Attachment #8763021 -
Flags: review?(arai.unmht) → review+
Updated•8 years ago
|
Attachment #8763022 -
Flags: review?(arai.unmht) → review+
Assignee | ||
Comment 23•8 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #21) > Any reason not to use Array.prototype.pop and push? I think Array.prototype.pop, cached, could be used. I did it this way to be absolutely clear about its having no side effects, other than length-consultation and modification if necessary. That's not necessarily so clear for a mutating, builtin function. Array.prototype.push isn't used because it *sets* the element to append it to the array, rather than defining it. So it's vulnerable to a setter on Array.prototype or Object.prototype making behavior undependable. This is a little odd -- because I think we get this wrong now. Chrome, at least, correctly prints with this testcase: Object.defineProperty(Array.prototype, 0, { set: function(v) { print('hi'); } }); [].push(5); But we don't seem to in Firefox (and I think not in a shell build, although I'm having trouble making one right now because of build nonsense). I don't know if we have a bug on file for this yet; I'll try to find one. I'm tryservering everything up to (but not including) this patch to land it first, separately, so you have a little time before I get to the rest of the patches here, if you have any observations you want to make. :-)
Flags: needinfo?(jwalden+bmo)
Comment 24•8 years ago
|
||
Comment on attachment 8763014 [details] [diff] [review] Move callstack-recording functionality into shell.js's IIFE Review of attachment 8763014 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for clarification :) In that case, it would be nice to have some comment about the reason why re-implementing push/pop there, so that no one won't re-write it with cached Array.prototype.push in future.
Attachment #8763014 -
Flags: review?(arai.unmht) → review+
Comment 25•8 years ago
|
||
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/7d0192747331 Begin to make jstests' shell.js/browser.js less dependent on tests being well-behaved, and make exporting functionality a more deliberate choice. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/94cec7dd2981 Move newGlobal's browser polyfill into shell.js, as the first step of unifying shell.js/browser.js. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ffca66a5b6 Move DocumentWrite into browser.js's IIFE. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/3e2d92d20513 Move assertThrowsInstanceOf into shell.js's IIFE. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/11cea69b0788 Remove a gazillion useless jit(true) and jit(false) calls from jstests. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/b179ec85b1c0 Manually remove the remaining jit() call oddballs. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/f4b09cf811dd Remove jit() fully from shell.js/browser.js. r=arai
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Comment 26•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7d0192747331 https://hg.mozilla.org/mozilla-central/rev/94cec7dd2981 https://hg.mozilla.org/mozilla-central/rev/c0ffca66a5b6 https://hg.mozilla.org/mozilla-central/rev/3e2d92d20513 https://hg.mozilla.org/mozilla-central/rev/11cea69b0788 https://hg.mozilla.org/mozilla-central/rev/b179ec85b1c0 https://hg.mozilla.org/mozilla-central/rev/f4b09cf811dd
Comment 27•8 years ago
|
||
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/bbde749db2bd Move callstack-recording functionality into shell.js's IIFE. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/056efc0848c3 Move assertEqArray into shell.js's IIFE, and delete a duplicate of it elsewhere. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/d3ab52f7cf76 Remove BigO from shell.js, and remove the one (disabled) test that used it. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/4e274e923823 Make jsref.js non-special as far as harnesses are concerned, and remove all jsref.js files as unused. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/c003801a25c1 Remove the dead getFailedCases shell.js function. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/cbf85303aeb2 Move Permutations into ecma_6/shell.js, out of the central shell.js. r=arai
Comment 28•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bbde749db2bd https://hg.mozilla.org/mozilla-central/rev/056efc0848c3 https://hg.mozilla.org/mozilla-central/rev/d3ab52f7cf76 https://hg.mozilla.org/mozilla-central/rev/4e274e923823 https://hg.mozilla.org/mozilla-central/rev/c003801a25c1 https://hg.mozilla.org/mozilla-central/rev/cbf85303aeb2
Comment 29•8 years ago
|
||
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/d9d62360b963 Remove the unnecessary VERBOSE variable from shell.js. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/90582bb8f7ed Remove a never-observed |var DEBUG = false;|. r=arai
Comment 30•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d9d62360b963 https://hg.mozilla.org/mozilla-central/rev/90582bb8f7ed
Assignee | ||
Comment 31•8 years ago
|
||
In the longer run, everything should be in shell.js. For now, I am going to be somewhat inconsistent about whether cleanups in browser.js move functionality into shell.js, or simply into browser.js's IIFE. Once everything's in IIFEs, moving stuff out of browser.js should be easier. In the meantime, and especially for functionality that other non-IIFE functions in browser.js use, I somewhat prefer leaving the functionality definition in browser.js, rather than move it to shell.js and have to export it.
Attachment #8777878 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 32•8 years ago
|
||
Attachment #8777879 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 33•8 years ago
|
||
Obviously we should be a bit leery about changing the main harness HTML file. But there's not really any other way to do this, than to move things around. Fortunately, jstests rarely directly interact with the harness page.
Attachment #8777880 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 34•8 years ago
|
||
Attachment #8777881 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 35•8 years ago
|
||
Attachment #8777882 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 36•8 years ago
|
||
Some non-IIFE'd browser.js code still uses document.write, but this gets rid of all the code that had to do faux HTML escaping, which is a big win.
Attachment #8777883 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 37•8 years ago
|
||
This one took awhile to get done, with all the preliminary patches.
Attachment #8777884 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 38•8 years ago
|
||
Attachment #8777885 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 39•8 years ago
|
||
Attachment #8777886 -
Flags: review?(arai.unmht)
Comment 40•8 years ago
|
||
Comment on attachment 8777878 [details] [diff] [review] Define gczeal/quit in shell.js, out of browser.js, plus a few other minor changes Review of attachment 8777878 [details] [diff] [review]: ----------------------------------------------------------------- Thanks :) will review remaining patches tomorrow.
Attachment #8777878 -
Flags: review?(arai.unmht) → review+
Updated•8 years ago
|
Attachment #8777879 -
Flags: review?(arai.unmht) → review+
Updated•8 years ago
|
Attachment #8777880 -
Flags: review?(arai.unmht) → review+
Updated•8 years ago
|
Attachment #8777881 -
Flags: review?(arai.unmht) → review+
Comment 41•8 years ago
|
||
Comment on attachment 8777882 [details] [diff] [review] Make writeFormattedResult write into the page in a way that never sets innerHTML or uses document.write Review of attachment 8777882 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/tests/browser.js @@ +158,5 @@ > + AppendChild(b, font); > + > + var tt = CreateElement("tt"); > + SetTextContent(tt, string); > + AppendChild(tt, b); I hope we can replace those font/b/tt elements' effect with CSS and move those styling into HTML file. (not in this patch. later patch or separated bug)
Attachment #8777882 -
Flags: review?(arai.unmht) → review+
Updated•8 years ago
|
Attachment #8777883 -
Flags: review?(arai.unmht) → review+
Comment 42•8 years ago
|
||
Comment on attachment 8777884 [details] [diff] [review] Define and export print and dump functions from shell.js's IIFE *only* Review of attachment 8777884 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/tests/shell.js @@ +153,5 @@ > /**************************** > * UTILITY FUNCTION EXPORTS * > ****************************/ > > + var dump = global.dump; I was about to ask this for previous patch. good to see this here :) @@ +167,5 @@ > + // no-op. > + dump = function() {}; > + } else { > + // |print| prints to stdout: make |dump| do likewise. > + dump = global.print; |dump| doesn't put newline at the end, but |print| does. so this will put extra newline for standard use-case of |dump|, like: dump("hello\n"); it won't matter much here tho...
Attachment #8777884 -
Flags: review?(arai.unmht) → review+
Updated•8 years ago
|
Attachment #8777885 -
Flags: review?(arai.unmht) → review+
Updated•8 years ago
|
Attachment #8777886 -
Flags: review?(arai.unmht) → review+
Comment 43•8 years ago
|
||
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/2b22fd600973 Define gczeal/quit in shell.js, out of browser.js, plus a few other minor changes. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/00936e6b524e Make browser.js's newGlobal and DocumentWrite work regardless of the test's behavior. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/4862d126b246 Make browser.js's print() function write into the page in a way that never sets innerHTML or uses document.write. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/cf48cc23269d Make browser.js's writeHeaderToLog write into the page in a way that never sets innerHTML or uses document.write. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/df10776fd165 Make writeFormattedResult write into the page in a way that never sets innerHTML or uses document.write. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/c2a3429db6cb Remove DocumentWrite and htmlesc as unused. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/e96d2fb8ccc7 Define and export print and dump functions from shell.js's IIFE *only*. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/e9a6e4989147 Remove the pointless stopTest function from shell.js. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/6a03ccc7c02e Remove browser.js's never-used include() function. r=arai
Comment 44•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2b22fd600973 https://hg.mozilla.org/mozilla-central/rev/00936e6b524e https://hg.mozilla.org/mozilla-central/rev/4862d126b246 https://hg.mozilla.org/mozilla-central/rev/cf48cc23269d https://hg.mozilla.org/mozilla-central/rev/df10776fd165 https://hg.mozilla.org/mozilla-central/rev/c2a3429db6cb https://hg.mozilla.org/mozilla-central/rev/e96d2fb8ccc7 https://hg.mozilla.org/mozilla-central/rev/e9a6e4989147 https://hg.mozilla.org/mozilla-central/rev/6a03ccc7c02e
Updated•7 years ago
|
Keywords: triage-deferred
Priority: -- → P3
Comment 45•6 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months. :Waldo, maybe it's time to close this bug?
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 46•6 years ago
|
||
I'd really like to combine/unify shell.js and browser.js into one still, but it *does* appear the two files do everything in IIFEs. (If three of them, because browser.js has two for some reason.) Let's call this done and deal with the rest in other bugs, whenever someone gets has reason to deal with that.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: needinfo?(jwalden+bmo)
Resolution: --- → FIXED
Updated•6 years ago
|
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•