Closed
Bug 1346234
Opened 7 years ago
Closed 7 years ago
Remove more junk from jsreftest helpers
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla58
People
(Reporter: anba, Assigned: anba)
Details
(Keywords: triage-deferred)
Attachments
(46 files, 45 obsolete files)
... and further encapsulate internal test harness functions.
Assignee | ||
Comment 1•7 years ago
|
||
Removes "gTestcases" from comments in preparation for parts 2-3.
Assignee | ||
Comment 2•7 years ago
|
||
Remove direct access to the global shell.js variables "gTestcases" and "gTc" from tests.
Assignee | ||
Comment 3•7 years ago
|
||
Make the test cases array a local variable in shell's IIFE. Also stop exporting writeTestCaseResult because it's no longer accessed after the changes in part 2.
Assignee | ||
Comment 4•7 years ago
|
||
writeFormattedResult is only called when |isReftest| is false, which should imply it's only called for the shell runner. That means we can remove the override in browser.js and inline the function in writeTestCaseResult.
Assignee | ||
Comment 5•7 years ago
|
||
It's probably safe to assume that |isReftest| is true iff |runningInBrowser| is true. Or is there any other browser-based runner which doesn't use jsreftest.html?
Assignee | ||
Comment 6•7 years ago
|
||
|PASSED| and |FAILED| can be changed to local variables in shell.js. And reportCompare isn't overwritten in browser.js, so we don't need the |global.reportCompare| indirection.
Assignee | ||
Comment 7•7 years ago
|
||
Inlines writeTestCaseResult and removes the try-catch workaround for js1.7
Assignee | ||
Comment 8•7 years ago
|
||
Avoid modifying test case result objects after construction, so we can later stop exposing the test case objects.
Assignee | ||
Comment 9•7 years ago
|
||
Removes the duplicate call to getTestCaseResult() to set the |.passed| property of test case objects. (And moves getTestCaseResult() closer to its only call site.)
Assignee | ||
Comment 10•7 years ago
|
||
It's not needed to set the global |EXPECTED| variable when the test file is already marked as negative (= the file name ends with "-n.js").
Assignee | ||
Comment 11•7 years ago
|
||
Allows to set the |.reason| property of test cases in the constructor. With this change the test case objects are no longer modified in test files.
Assignee | ||
Comment 12•7 years ago
|
||
Removes return value from reportCompare and reportMatch. And fixes multiple issues in js1_8_1/jit/math-jit-tests.js (code duplication in close_enough; +0/-0 detection which didn't actually worked because it's still calling shell.js's reportCompare function; direct assignment of the |.name| property of a function, etc.).
Assignee | ||
Comment 13•7 years ago
|
||
TestCase.prototype.dump is a no-op in browser runs, so stop calling it in jsTestDriverEnd().
Assignee | ||
Comment 14•7 years ago
|
||
Just fix the test case instead of marking it as "|reftest| fails".
Assignee | ||
Comment 15•7 years ago
|
||
No longer export toPrinted from shell.js, instead provide a minimum replacement to the one test file which called it. Also fixes a typo I introduced in a different bug.
Assignee | ||
Comment 16•7 years ago
|
||
Remove the global |STATUS| variable from shell.js
Assignee | ||
Comment 17•7 years ago
|
||
Tests don't call |currentFunc()| so we don't need to export it.
Assignee | ||
Comment 18•7 years ago
|
||
Use template strings for readability. And add a helper |newReportCompareTestCase| to reduce code duplication in |reportCompare| and |reportMatch|.
Assignee | ||
Comment 19•7 years ago
|
||
enterFunc/exitFunc are never called with a function name containing "()", so we can remove the extra processing in enterFunc/exitFunc and just add "()" in |currentFunc|.
Assignee | ||
Comment 20•7 years ago
|
||
Remove feature guards for Promises because they're enabled by default.
Assignee | ||
Comment 21•7 years ago
|
||
Remove a no longer needed workaround for lexical environments.
Assignee | ||
Comment 22•7 years ago
|
||
It's no longer necessary to call options('strict') or options('werror') to enable syntax errors for variables using keywords as names.
Assignee | ||
Comment 23•7 years ago
|
||
Calling |startTest()| is a no-op when |BUGNUMBER| isn't set, so remove all |startTest()| calls when |BUGNUMBER| wasn't set earlier.
Assignee | ||
Comment 24•7 years ago
|
||
Remove uses of |BUGNUMBER| when no actual bug number is set.
Assignee | ||
Comment 25•7 years ago
|
||
Replace the remaining calls to |startTest()| with |printBugNumber(BUGNUMBER)|, because they're equivalent after part 23.
Assignee | ||
Comment 26•7 years ago
|
||
|startTest| is no longer used, so let's remove it from shell.js
Assignee | ||
Comment 27•7 years ago
|
||
Remove calls to enter/exitFunc when there's only a single test case function. (In preparation for complete removal of enter/exitFunc.)
Assignee | ||
Comment 28•7 years ago
|
||
Only a single test was actually using the enter/exitFunc feature in a useful way, but it's not worth it to keep this feature for one test.
Assignee | ||
Comment 29•7 years ago
|
||
Actual removal of enter/exitFunc from shell.js
Assignee | ||
Comment 30•7 years ago
|
||
Avoid calling |quit()| in tests because it doesn't do what you expect to do in the browser runner.
Assignee | ||
Comment 31•7 years ago
|
||
Move the global |GLOBAL| variable to the tests which actually use it.
Assignee | ||
Comment 32•7 years ago
|
||
|optionsPop| is never called, so we can remove it. But then we also don't need to have |optionsPush|, so let's remove it, too.
Assignee | ||
Comment 33•7 years ago
|
||
|optionsReset| is effectively a no-op, because it uses an incorrect hasOwnProperty() check (it should be |options.initvalues.hasOwnProperty| instead of |options.hasOwnProperty|). We could fix this bug or alternatively we could just remove |optionsReset| completely because it doesn't seem to be used. Let's do the latter!
Assignee | ||
Comment 34•7 years ago
|
||
Moves the options helper functions into the IIFE and does some minor clean-up to make the initial options processing more efficient (because this code is called during test start-up).
Assignee | ||
Comment 35•7 years ago
|
||
shell.js always initialized a global |dump| function, so the |typeof dump != 'function'| is never true in jsTestDriverBrowserInit(). And |gFailureExpected| is never used, so we can remove it, too.
Assignee | ||
Comment 36•7 years ago
|
||
The test case object has an unused |.name| property, let's remove it, too. This will also allow us to remove the global |SECTION| variable in the next part.
Assignee | ||
Comment 37•7 years ago
|
||
Removes the global |SECTION| variable from shell.js, because it's no longer used.
Updated•7 years ago
|
Keywords: triage-deferred
Priority: -- → P3
Assignee | ||
Comment 38•7 years ago
|
||
Update part 18 to apply cleanly on inbound.
Attachment #8846032 -
Attachment is obsolete: true
Assignee | ||
Comment 39•7 years ago
|
||
Update part 24 to apply cleanly on inbound.
Attachment #8846038 -
Attachment is obsolete: true
Assignee | ||
Comment 40•7 years ago
|
||
Update part 27 to apply cleanly on inbound.
Attachment #8846041 -
Attachment is obsolete: true
Assignee | ||
Comment 41•7 years ago
|
||
Update part 30 to apply cleanly on inbound.
Attachment #8846045 -
Attachment is obsolete: true
Assignee | ||
Comment 42•7 years ago
|
||
Update part 36 to apply cleanly on inbound.
Attachment #8846052 -
Attachment is obsolete: true
Assignee | ||
Comment 43•7 years ago
|
||
Missing notes for parts 2, 18, 19, and 30: Part 2: - The call to "addTestCase()" and then to invoke the Test API "test()" function pattern is also used in other tests: - https://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/js/src/tests/ecma/Date/15.9.5.10-4.js#27,40-51 - https://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/js/src/tests/shell.js#661-677 Part 18: - "newReportCompareTestCase" is kind of a bad name, I guess I planned to either unify reportCompare and reportMatch, or maybe even remove reportMatch in favour of reportCompare? Part 19: - enterFunc/exitFunc actually get removed in a later patch (part 29) Part 30: - The SharedArrayBuffer and Atomics guards are probably no longer necessary.
Assignee | ||
Updated•7 years ago
|
Attachment #8845973 -
Flags: review?(sphink)
Assignee | ||
Comment 44•7 years ago
|
||
Comment on attachment 8845974 [details] [diff] [review] bug1346234-part2-gtestcases-access.patch See comment #43 about the "addTestCase" pattern.
Attachment #8845974 -
Flags: review?(sphink)
Assignee | ||
Updated•7 years ago
|
Attachment #8845994 -
Flags: review?(sphink)
Assignee | ||
Updated•7 years ago
|
Attachment #8845997 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8846000 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8846004 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8846005 -
Flags: review?(sphink)
Assignee | ||
Updated•7 years ago
|
Attachment #8846006 -
Flags: review?(sphink)
Assignee | ||
Updated•7 years ago
|
Attachment #8846007 -
Flags: review?(sphink)
Assignee | ||
Updated•7 years ago
|
Attachment #8846008 -
Flags: review?(sphink)
Assignee | ||
Updated•7 years ago
|
Attachment #8846011 -
Flags: review?(sphink)
Assignee | ||
Updated•7 years ago
|
Attachment #8846025 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8846026 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8846027 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8846029 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8846030 -
Flags: review?(sphink)
Assignee | ||
Updated•7 years ago
|
Attachment #8846031 -
Flags: review?(sphink)
Assignee | ||
Updated•7 years ago
|
Attachment #8921974 -
Flags: review?(sphink)
Assignee | ||
Comment 45•7 years ago
|
||
Comment on attachment 8846033 [details] [diff] [review] bug1346234-part19-enterexitfunc-paren-check.patch enterFunc and exitFunc will actually be removed later (see comment #43)!
Attachment #8846033 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8846034 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8846035 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8846036 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8846037 -
Flags: review?(sphink)
Assignee | ||
Updated•7 years ago
|
Attachment #8921975 -
Flags: review?(sphink)
Assignee | ||
Updated•7 years ago
|
Attachment #8846039 -
Flags: review?(sphink)
Assignee | ||
Updated•7 years ago
|
Attachment #8846040 -
Flags: review?(sphink)
Assignee | ||
Updated•7 years ago
|
Attachment #8921976 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8846043 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8846044 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8921977 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8846046 -
Flags: review?(sphink)
Assignee | ||
Updated•7 years ago
|
Attachment #8846047 -
Flags: review?(sphink)
Assignee | ||
Updated•7 years ago
|
Attachment #8846049 -
Flags: review?(sphink)
Assignee | ||
Updated•7 years ago
|
Attachment #8846050 -
Flags: review?(sphink)
Assignee | ||
Updated•7 years ago
|
Attachment #8846051 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8921978 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8846053 -
Flags: review?(jwalden+bmo)
Updated•7 years ago
|
Attachment #8845997 -
Flags: review?(jwalden+bmo) → review+
Comment 46•7 years ago
|
||
Comment on attachment 8846000 [details] [diff] [review] bug1346234-part5-remove-dupl-env-type-vars.patch Review of attachment 8846000 [details] [diff] [review]: ----------------------------------------------------------------- Seems reasonable enough to rely on, in the interests of simplicity.
Attachment #8846000 -
Flags: review?(jwalden+bmo) → review+
Updated•7 years ago
|
Attachment #8846004 -
Flags: review?(jwalden+bmo) → review+
Comment 47•7 years ago
|
||
Comment on attachment 8846025 [details] [diff] [review] bug1346234-part12-return-val-report-functions.patch Review of attachment 8846025 [details] [diff] [review]: ----------------------------------------------------------------- A little rs-y on this one, but at the worst it would be easier to fix the much-smaller post-change test if there were some bug.
Attachment #8846025 -
Flags: review?(jwalden+bmo) → review+
Updated•7 years ago
|
Attachment #8846026 -
Flags: review?(jwalden+bmo) → review+
Comment 48•7 years ago
|
||
Comment on attachment 8846027 [details] [diff] [review] bug1346234-part14-fix-whitespace-fails-tests.patch Review of attachment 8846027 [details] [diff] [review]: ----------------------------------------------------------------- lol
Attachment #8846027 -
Flags: review?(jwalden+bmo) → review+
Updated•7 years ago
|
Attachment #8846029 -
Flags: review?(jwalden+bmo) → review+
Updated•7 years ago
|
Attachment #8846033 -
Flags: review?(jwalden+bmo) → review+
Updated•7 years ago
|
Attachment #8846034 -
Flags: review?(jwalden+bmo) → review+
Updated•7 years ago
|
Attachment #8846035 -
Flags: review?(jwalden+bmo) → review+
Comment 49•7 years ago
|
||
Comment on attachment 8846036 [details] [diff] [review] bug1346234-part22-unnecessary-options-calls.patch Review of attachment 8846036 [details] [diff] [review]: ----------------------------------------------------------------- I hope we can get rid of |options| as a frob used in tests at all at some point. Its necessary statefulness is a real pain to work with.
Attachment #8846036 -
Flags: review?(jwalden+bmo) → review+
Updated•7 years ago
|
Attachment #8921976 -
Flags: review?(jwalden+bmo) → review+
Updated•7 years ago
|
Attachment #8846043 -
Flags: review?(jwalden+bmo) → review+
Updated•7 years ago
|
Attachment #8846044 -
Flags: review?(jwalden+bmo) → review+
Comment 50•7 years ago
|
||
Comment on attachment 8921977 [details] [diff] [review] bug1346234-part30-remove-quit-calls.patch Review of attachment 8921977 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/tests/Intl/extensions/options-value-emulates-undefined.js @@ +16,5 @@ > > var opt = createIsHTMLDDA(); > opt.toString = function() { return "long"; }; > > var str = new Date(2013, 12 - 1, 14).toLocaleString("en-US", { weekday: opt }); Hmm, this test probably can be upstreamed if we make it test resolvedOptions() rather than look at actual output. ::: js/src/tests/shell.js @@ -209,5 @@ > - var quit = global.quit; > - if (typeof quit !== "function") { > - // XXX There's something very strange about quit() in browser runs being a > - // function that doesn't quit at all (!). We should get rid of quit() > - // as an integral part of tests in favor of something else. \o/
Attachment #8921977 -
Flags: review?(jwalden+bmo) → review+
Updated•7 years ago
|
Attachment #8846051 -
Flags: review?(jwalden+bmo) → review+
Comment 51•7 years ago
|
||
Comment on attachment 8921978 [details] [diff] [review] bug1346234-part36-remove-testcase-name-property.patch Review of attachment 8921978 [details] [diff] [review]: ----------------------------------------------------------------- Unlike the other big review earlier, I timed out on rapid-PgDning this, so just rs=me.
Attachment #8921978 -
Flags: review?(jwalden+bmo) → review+
Comment 52•7 years ago
|
||
(I did look at the shell.js bit of it specifically, tho.)
Updated•7 years ago
|
Attachment #8846053 -
Flags: review?(jwalden+bmo) → review+
Updated•7 years ago
|
Attachment #8845973 -
Flags: review?(sphink) → review+
Updated•7 years ago
|
Attachment #8845974 -
Flags: review?(sphink) → review+
Updated•7 years ago
|
Attachment #8845994 -
Flags: review?(sphink) → review+
Updated•7 years ago
|
Attachment #8846005 -
Flags: review?(sphink) → review+
Comment 53•7 years ago
|
||
Comment on attachment 8846006 [details] [diff] [review] bug1346234-part8-no-modify-after-construction.patch Review of attachment 8846006 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/tests/ecma/Math/15.8.2.14.js @@ +39,5 @@ > var testcase = new TestCase( SECTION, > "Math.random()", > "pass", > + actual ); > + testcase.reason = result; There's some trailing whitespace that could be trimmed here. And I guess the testcase.reason setting will be dealt with at another time/patch?
Attachment #8846006 -
Flags: review?(sphink) → review+
Updated•7 years ago
|
Attachment #8846007 -
Flags: review?(sphink) → review+
Updated•7 years ago
|
Attachment #8846008 -
Flags: review?(sphink) → review+
Comment 54•7 years ago
|
||
Comment on attachment 8846011 [details] [diff] [review] bug1346234-part11-testcase-reason-argument.patch Review of attachment 8846011 [details] [diff] [review]: ----------------------------------------------------------------- Ah, here it is. ::: js/src/tests/ecma/Math/15.8.2.14.js @@ +40,4 @@ > "Math.random()", > "pass", > + actual, > + result ); The indentation appears to be wrong here. (And there's trailing whitespace -- maybe this is fixed globally in some other patch?)
Attachment #8846011 -
Flags: review?(sphink) → review+
Updated•7 years ago
|
Attachment #8846030 -
Flags: review?(sphink) → review+
Updated•7 years ago
|
Attachment #8846031 -
Flags: review?(sphink) → review+
Updated•7 years ago
|
Attachment #8846037 -
Flags: review?(sphink) → review+
Updated•7 years ago
|
Attachment #8846039 -
Flags: review?(sphink) → review+
Updated•7 years ago
|
Attachment #8846040 -
Flags: review?(sphink) → review+
Updated•7 years ago
|
Attachment #8846046 -
Flags: review?(sphink) → review+
Updated•7 years ago
|
Attachment #8846047 -
Flags: review?(sphink) → review+
Updated•7 years ago
|
Attachment #8846049 -
Flags: review?(sphink) → review+
Comment 55•7 years ago
|
||
Comment on attachment 8846050 [details] [diff] [review] bug1346234-part34-more-options-cleanup.patch Review of attachment 8846050 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review so anba can educate me about how this harness thing works, but tagging f? just because there are a lot of patches here and I don't want it to get lost. ::: js/src/tests/browser.js @@ +340,5 @@ > + var value = ""; > + for (var optionName in options.currvalues) { > + if (value) > + value += ","; > + value += optionName; How about var initialOptions = Object.keys(options.currvalues).join(","); (or 'const', if we're ok using that here. I guess it doesn't fit in with the existing archaic style.) @@ +372,5 @@ > jstestsOptions = options; > > +function optionsInitAndClear() { > + // Hash containing the set options. > + options.currvalues = {__proto__: null}; This is fine, but I'm more used to seeing Object.create(null). I notice that our engine internally treats them a little differently, though (which iiuc should only matter for performance; I'm just curious.) @@ +377,5 @@ > > + // Turn off current settings. > + var allowedOptions = ["strict", "werror", "strict_mode"]; > + for (var i = 0; i < allowedOptions.length; ++i) { > + var optionName = allowedOptions[i]; Is this script trying to maintain backwards-compatibility? I would expect for (const optionName of ["strict", "werror", "strict_mode"]) ::: js/src/tests/shell.js @@ +579,5 @@ > > + // Harness internal function, only exported for browser.js. > + // XXX: This function is only exported for the window.onerror handler in > + // browser.js. If the handler doesn't actually need to clear the options, we > + // can remove this export. Ok, I don't really understand how this harness works in the first place. How does browser.js needing something mean it needs to be exported in shell.js? Is it just that we want to keep the set of exports the same? But if so, why bother implementing a body here at all? /me is confused @@ +593,5 @@ > + var optionName = optionNames[i]; > + if (optionName) { > + options(optionName); > + } > + } Maybe this is packing too much in, but personally I'd find this easier to follow as for (const optionName of currentOptions.split(',')) { if (optionName) options(optionName); } though I'm kind of curious how an empty option name can creep in, given that we know currentOptions !== "".
Attachment #8846050 -
Flags: review?(sphink) → feedback?(andrebargull)
Comment 56•7 years ago
|
||
Comment on attachment 8921974 [details] [diff] [review] bug1346234-part18-templ-string-err-messages.patch Review of attachment 8921974 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/tests/shell.js @@ +469,5 @@ > // often don't. This seems unimportant. > return true; > } > > + function newReportCompareTestCase(description, expected, actual, output) { Kind of a weird name, especially since it contains "ReportCompare" but is used by reportMatch. reportTestCaseResult?
Attachment #8921974 -
Flags: review?(sphink) → review+
Updated•7 years ago
|
Attachment #8921975 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 57•7 years ago
|
||
(In reply to Steve Fink [:sfink] [:s:] from comment #55) > ::: js/src/tests/browser.js > @@ +340,5 @@ > > + var value = ""; > > + for (var optionName in options.currvalues) { > > + if (value) > > + value += ","; > > + value += optionName; > > How about > > var initialOptions = Object.keys(options.currvalues).join(","); > > (or 'const', if we're ok using that here. I guess it doesn't fit in with the > existing archaic style.) jstests is also used to run test262, and some test262 tests may or may not remove or alter built-in functionality, so it's safer not to rely on built-ins (here: Object.keys and String.prototype.join) still being present after test code has run. So what we did in the past, is to reduce the globals introduced by the test harness by moving most of the harness code into an IIFE and to save built-in functions when we start the test: https://searchfox.org/mozilla-central/rev/dd47bee6468de7e1221b4d006342ad6b9813d0e5/js/src/tests/shell.js#6-10,15-17 This work isn't yet completely done, we still need to move more harness code into the IIFEs and to save more built-ins, see for example bug 1346080. :-/ > > @@ +372,5 @@ > > jstestsOptions = options; > > > > +function optionsInitAndClear() { > > + // Hash containing the set options. > > + options.currvalues = {__proto__: null}; > > This is fine, but I'm more used to seeing Object.create(null). I notice that > our engine internally treats them a little differently, though (which iiuc > should only matter for performance; I'm just curious.) That one should be okay to change. > > @@ +377,5 @@ > > > > + // Turn off current settings. > > + var allowedOptions = ["strict", "werror", "strict_mode"]; > > + for (var i = 0; i < allowedOptions.length; ++i) { > > + var optionName = allowedOptions[i]; > > Is this script trying to maintain backwards-compatibility? I would expect > > for (const optionName of ["strict", "werror", "strict_mode"]) A test could have removed Array.prototype[Symbol.iterator], so it's safer to use a normal C-style for-loop. > > ::: js/src/tests/shell.js > @@ +579,5 @@ > > > > + // Harness internal function, only exported for browser.js. > > + // XXX: This function is only exported for the window.onerror handler in > > + // browser.js. If the handler doesn't actually need to clear the options, we > > + // can remove this export. > > Ok, I don't really understand how this harness works in the first place. How > does browser.js needing something mean it needs to be exported in shell.js? > Is it just that we want to keep the set of exports the same? But if so, why > bother implementing a body here at all? > > /me is confused When we run jstests in the shell, only the shell.js files are loaded. But when we run jstests in the browser, the shell.js _and_ the browser.js files are loaded. In this case, the "optionsClear" function is initially called before we start a test (as part of the start-up code in shell.js) and also in the window's onerror handler. The onerror handler is installed in browser.js, therefore "optionsClear" is exported from shell.js, so browser.js can access it, too. > > @@ +593,5 @@ > > + var optionName = optionNames[i]; > > + if (optionName) { > > + options(optionName); > > + } > > + } > > Maybe this is packing too much in, but personally I'd find this easier to > follow as > > for (const optionName of currentOptions.split(',')) { > if (optionName) > options(optionName); > } > > though I'm kind of curious how an empty option name can creep in, given that > we know currentOptions !== "". See above why for-of loops could be problematic.
Assignee | ||
Comment 58•7 years ago
|
||
Oh, it looks like the hooks for Spider are probably no longer necessary (bug 1396088).
Assignee | ||
Comment 59•7 years ago
|
||
(In reply to André Bargull [:anba] from comment #57) > > > > @@ +377,5 @@ > > > > > > + // Turn off current settings. > > > + var allowedOptions = ["strict", "werror", "strict_mode"]; > > > + for (var i = 0; i < allowedOptions.length; ++i) { > > > + var optionName = allowedOptions[i]; > > > > Is this script trying to maintain backwards-compatibility? I would expect > > > > for (const optionName of ["strict", "werror", "strict_mode"]) > > A test could have removed Array.prototype[Symbol.iterator], so it's safer to > use a normal C-style for-loop. > Except that in this case it's safe to use for-of, because the code is run before any test could have modified built-ins.
Assignee | ||
Comment 60•7 years ago
|
||
(In reply to Steve Fink [:sfink] [:s:] from comment #55) > ::: js/src/tests/shell.js > @@ +579,5 @@ > > > > + // Harness internal function, only exported for browser.js. > > + // XXX: This function is only exported for the window.onerror handler in > > + // browser.js. If the handler doesn't actually need to clear the options, we > > + // can remove this export. > > Ok, I don't really understand how this harness works in the first place. How > does browser.js needing something mean it needs to be exported in shell.js? > Is it just that we want to keep the set of exports the same? But if so, why > bother implementing a body here at all? > > /me is confused Ugh, you've successfully tricked me into spending more time to dissect how all this works which resulted into adding another ten patches. Thanks for that! :-p > @@ +593,5 @@ > > + var optionName = optionNames[i]; > > + if (optionName) { > > + options(optionName); > > + } > > + } > > Maybe this is packing too much in, but personally I'd find this easier to > follow as > > for (const optionName of currentOptions.split(',')) { > if (optionName) > options(optionName); > } > > though I'm kind of curious how an empty option name can creep in, given that > we know currentOptions !== "". Oh sure, the extra if-statement isn't necessary. Thanks for spotting that!
Assignee | ||
Comment 61•7 years ago
|
||
Attachment #8845973 -
Attachment is obsolete: true
Attachment #8923384 -
Flags: review+
Assignee | ||
Comment 62•7 years ago
|
||
Attachment #8845974 -
Attachment is obsolete: true
Attachment #8923387 -
Flags: review+
Assignee | ||
Comment 63•7 years ago
|
||
Attachment #8845994 -
Attachment is obsolete: true
Attachment #8923388 -
Flags: review+
Assignee | ||
Comment 64•7 years ago
|
||
Attachment #8845997 -
Attachment is obsolete: true
Attachment #8923389 -
Flags: review+
Assignee | ||
Comment 65•7 years ago
|
||
Attachment #8846000 -
Attachment is obsolete: true
Attachment #8923390 -
Flags: review+
Assignee | ||
Comment 66•7 years ago
|
||
Attachment #8846004 -
Attachment is obsolete: true
Attachment #8923391 -
Flags: review+
Assignee | ||
Comment 67•7 years ago
|
||
Attachment #8923392 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Attachment #8846005 -
Attachment is obsolete: true
Assignee | ||
Comment 68•7 years ago
|
||
Attachment #8846006 -
Attachment is obsolete: true
Attachment #8923397 -
Flags: review+
Assignee | ||
Comment 69•7 years ago
|
||
Attachment #8846007 -
Attachment is obsolete: true
Attachment #8923398 -
Flags: review+
Assignee | ||
Comment 70•7 years ago
|
||
Attachment #8846008 -
Attachment is obsolete: true
Attachment #8923399 -
Flags: review+
Assignee | ||
Comment 71•7 years ago
|
||
Attachment #8846011 -
Attachment is obsolete: true
Attachment #8923400 -
Flags: review+
Assignee | ||
Comment 72•7 years ago
|
||
Attachment #8846025 -
Attachment is obsolete: true
Attachment #8923401 -
Flags: review+
Assignee | ||
Comment 73•7 years ago
|
||
Attachment #8846026 -
Attachment is obsolete: true
Attachment #8923402 -
Flags: review+
Assignee | ||
Comment 74•7 years ago
|
||
Attachment #8846027 -
Attachment is obsolete: true
Attachment #8923403 -
Flags: review+
Assignee | ||
Comment 75•7 years ago
|
||
Attachment #8846029 -
Attachment is obsolete: true
Attachment #8923404 -
Flags: review+
Assignee | ||
Comment 76•7 years ago
|
||
Attachment #8846030 -
Attachment is obsolete: true
Attachment #8923405 -
Flags: review+
Assignee | ||
Comment 77•7 years ago
|
||
Attachment #8846031 -
Attachment is obsolete: true
Attachment #8923406 -
Flags: review+
Assignee | ||
Comment 78•7 years ago
|
||
Renamed harness function to "reportTestCaseResult" per review comments.
Attachment #8921974 -
Attachment is obsolete: true
Attachment #8923407 -
Flags: review+
Assignee | ||
Comment 79•7 years ago
|
||
Attachment #8846033 -
Attachment is obsolete: true
Attachment #8923408 -
Flags: review+
Assignee | ||
Comment 80•7 years ago
|
||
Attachment #8846034 -
Attachment is obsolete: true
Attachment #8923409 -
Flags: review+
Assignee | ||
Comment 81•7 years ago
|
||
Attachment #8846035 -
Attachment is obsolete: true
Attachment #8923410 -
Flags: review+
Assignee | ||
Comment 82•7 years ago
|
||
Attachment #8846036 -
Attachment is obsolete: true
Attachment #8923411 -
Flags: review+
Assignee | ||
Comment 83•7 years ago
|
||
Attachment #8846037 -
Attachment is obsolete: true
Attachment #8923412 -
Flags: review+
Assignee | ||
Comment 84•7 years ago
|
||
Attachment #8921975 -
Attachment is obsolete: true
Attachment #8923413 -
Flags: review+
Assignee | ||
Comment 85•7 years ago
|
||
Attachment #8846039 -
Attachment is obsolete: true
Attachment #8923414 -
Flags: review+
Assignee | ||
Comment 86•7 years ago
|
||
Attachment #8846040 -
Attachment is obsolete: true
Attachment #8923415 -
Flags: review+
Assignee | ||
Comment 87•7 years ago
|
||
Attachment #8921976 -
Attachment is obsolete: true
Attachment #8923416 -
Flags: review+
Assignee | ||
Comment 88•7 years ago
|
||
Attachment #8846043 -
Attachment is obsolete: true
Attachment #8923417 -
Flags: review+
Assignee | ||
Comment 89•7 years ago
|
||
Attachment #8846044 -
Attachment is obsolete: true
Attachment #8923418 -
Flags: review+
Assignee | ||
Comment 90•7 years ago
|
||
Attachment #8921977 -
Attachment is obsolete: true
Attachment #8923419 -
Flags: review+
Assignee | ||
Comment 91•7 years ago
|
||
Attachment #8846046 -
Attachment is obsolete: true
Attachment #8923420 -
Flags: review+
Assignee | ||
Comment 92•7 years ago
|
||
Attachment #8846047 -
Attachment is obsolete: true
Attachment #8923421 -
Flags: review+
Assignee | ||
Comment 93•7 years ago
|
||
Attachment #8846049 -
Attachment is obsolete: true
Attachment #8923422 -
Flags: review+
Assignee | ||
Comment 94•7 years ago
|
||
For part 33: The original patch simply removed all callers to "optionsReset", but instead we need/should call "optionsClear" (which was called internally in "optionsReset").
Assignee | ||
Comment 95•7 years ago
|
||
Comment on attachment 8846050 [details] [diff] [review] bug1346234-part34-more-options-cleanup.patch Clearing f? for part 34, will add an updated patch and will post a new patch to further clean-up and hopefully also make it more understandable how the test harness initialization process works.
Attachment #8846050 -
Flags: feedback?(andrebargull)
Assignee | ||
Comment 96•7 years ago
|
||
More clean-ups for this part of the test framework are in part 40 and 43.
Attachment #8846050 -
Attachment is obsolete: true
Attachment #8923424 -
Flags: review?(sphink)
Assignee | ||
Comment 97•7 years ago
|
||
Attachment #8846051 -
Attachment is obsolete: true
Attachment #8923425 -
Flags: review+
Assignee | ||
Comment 98•7 years ago
|
||
Attachment #8921978 -
Attachment is obsolete: true
Attachment #8923427 -
Flags: review+
Assignee | ||
Comment 99•7 years ago
|
||
Attachment #8846053 -
Attachment is obsolete: true
Attachment #8923428 -
Flags: review+
Assignee | ||
Comment 100•7 years ago
|
||
Updated parts 1-37 to include the reviewer name in the commit message, carrying r+ where applicable.
Assignee | ||
Comment 101•7 years ago
|
||
Except for js1_5/Regress/regress-230216-2.js, EXPECTED is only set in negative test cases which also have a file name ending with "-n.js" and those test cases always pass when |window.onerror| is called (cf. |href.indexOf('-n.js') !== -1|). And the try-catch block in js1_5/Regress/regress-230216-2.js ensures that regress-230216-2.js won't ever invoke |window.onerror|. And that means the whole EXPECTED processing in |window.onerror| isn't in any test. While being there, I've additionally reordered some statements in |window.onerror| so that the |testcase.passed| property doesn't need to be modified manually.
Attachment #8923435 -
Flags: review?(sphink)
Assignee | ||
Comment 102•7 years ago
|
||
Removes the global |VERSION| variable from all tests and the test harness (the variable is never read/used).
Attachment #8923436 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 103•7 years ago
|
||
browser.js - |window.onerror| already calls |optionsClear| on entry, so it shouldn't be necessary to call it again when exiting the handler. - |optionsInit| initialized |options.currvalues| with the currently enabled options, and |optionsClear| then disabled all currently enabled options. It seems clearer to simply disable all set options directly in one step. shell.js - This code runs before browser.js is loaded, which means |options| is only a function at this point when running the tests in the shell. Changed the if-condition to make this more clear. - Also removed |gFailureExpected| again, because it seems I've accidently reverted its removal while tracking down a bug in an earlier patch.
Attachment #8923439 -
Flags: review?(sphink)
Assignee | ||
Comment 104•7 years ago
|
||
Only moves the remaining code in browser.js into the second IIFE, doesn't perform any clean-ups or changes in behaviour (see part 42).
Attachment #8923443 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 105•7 years ago
|
||
General clean-ups: - Changes single-quotes to double-quotes for consistency with the rest of browser.js. - Moves braces on the same line instead of the next line. - Changes == to === and != to !==. - Save/Use more saved built-ins to be more resilient against test overwriting built-in functions. window.onerror: - Moved the error type processing into jsTestDriverBrowserInit, so we don't have to worry about modified built-ins. - And because that enables us to use the existing properties processing code in jsTestDriverBrowserInit, so we don't need to inspect |document.location.href| in two different places. jsTestDriverBrowserInit: - The whole JavaScript version code will hopefully be gone in the next months...
Attachment #8923452 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 106•7 years ago
|
||
- Split |optionsClear| into a |shellOptionsClear| and a |browserOptionsClear| function, so we don't need to export and pollute the global namespace with |optionsClear|. - That also allows us to remove the code to restore the global "options" property in |window.onerror| and |jsTestDriverEnd|. (I guess this code was only added, because |optionsClear| reads the global "options" property, so it's necessary to restore |options| before being able to call |optionsClear|.) - The try-catch block in both |jsTestDriverEnd| functions seems unnecessary with the new |shellOptionsClear| and |browserOptionsClear| functions, because I don't see how an error can be thrown now. - Also save the built-in |options| function in shell.js, so we don't need to worry about tests modifying the global "options" property. - Same goes with storing the enabled options as |currentOptions| in browser.js. - And we can also make |browserOptionsClear| faster by directly iterating over |currentOptions|.
Attachment #8923455 -
Flags: review?(sphink)
Assignee | ||
Comment 107•7 years ago
|
||
- Add |skip-if(xulRuntime.shell)| for browser-only tests. - If the test was already |skip-if(!xulRuntime.shell)|, change it to just |skip| (maybe we should instead remove the complete test...). - And then remove all browser detection code now that the test is never run in the shell environment.
Attachment #8923456 -
Flags: review?(sphink)
Assignee | ||
Comment 108•7 years ago
|
||
- In part 44 we disabled all tests calling |jsTestDriverEnd| when running in the shell environment (these are all browser-only tests). - So now |jsTestDriverEnd| is definitely only called for browser tests, which means we can remove the |jsTestDriverEnd| function from shell.js and move |gDelayTestDriverEnd| to browser.js. (Actually shell.js' |jsTestDriverEnd| was never called to begin with, only now this is more easy to see.) - The |jsTestDriverEnd| function in shell.js was the only caller for |TestCase.prototype.dump|, now that |jsTestDriverEnd| is removed from shell.js, we can also remove |TestCase.prototype.dump|. - |TestCase.bugnumber| and |TestCase.type| were only used in |TestCase.prototype.dump|, and now that ... you see where this is going. :-)
Attachment #8923458 -
Flags: review?(sphink)
Assignee | ||
Comment 109•7 years ago
|
||
No more test metadata exports of dubious value. :-) (I've only added BUGNUMBER variables in some tests so it's easier to grep for them if we ever decide to remove from all tests.)
Attachment #8923460 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 110•7 years ago
|
||
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1eb5036d10f14189ce389b2fc36c0bf1d22c2067
Comment 111•7 years ago
|
||
(In reply to André Bargull [:anba] from comment #57) > (In reply to Steve Fink [:sfink] [:s:] from comment #55) > > ::: js/src/tests/browser.js > > @@ +340,5 @@ > > > + var value = ""; > > > + for (var optionName in options.currvalues) { > > > + if (value) > > > + value += ","; > > > + value += optionName; > > > > How about > > > > var initialOptions = Object.keys(options.currvalues).join(","); > > > > (or 'const', if we're ok using that here. I guess it doesn't fit in with the > > existing archaic style.) > > jstests is also used to run test262, and some test262 tests may or may not > remove or alter built-in functionality, so it's safer not to rely on > built-ins (here: Object.keys and String.prototype.join) still being present > after test code has run. So what we did in the past, is to reduce the > globals introduced by the test harness by moving most of the harness code > into an IIFE and to save built-in functions when we start the test: > https://searchfox.org/mozilla-central/rev/ > dd47bee6468de7e1221b4d006342ad6b9813d0e5/js/src/tests/shell.js#6-10,15-17 Ah! Thanks, that was what I was missing.
Updated•7 years ago
|
Attachment #8923424 -
Flags: review?(sphink) → review+
Updated•7 years ago
|
Attachment #8923435 -
Flags: review?(sphink) → review+
Updated•7 years ago
|
Attachment #8923439 -
Flags: review?(sphink) → review+
Comment 112•7 years ago
|
||
Comment on attachment 8923455 [details] [diff] [review] bug1346234-part43-remove-optionsClear-export.patch Review of attachment 8923455 [details] [diff] [review]: ----------------------------------------------------------------- Very nice. Removes a bunch of action-at-a-distance intertwingling.
Attachment #8923455 -
Flags: review?(sphink) → review+
Comment 113•7 years ago
|
||
Comment on attachment 8923456 [details] [diff] [review] bug1346234-part44-skip-browseronly-tests-in-shell.patch Review of attachment 8923456 [details] [diff] [review]: ----------------------------------------------------------------- \o/
Attachment #8923456 -
Flags: review?(sphink) → review+
Updated•7 years ago
|
Attachment #8923458 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 114•7 years ago
|
||
And some day we should probably also look into reducing the amount of unnecessary print output from some tests, because I doubt there's much value to fill the logs with "Apfelkiste, Apfelschale", unless we try to teach German to the CI servers. :-) https://searchfox.org/mozilla-central/rev/1ebd2eff44617df3b82eea7d2f3ca1b60cc591a0/js/src/tests/js1_5/Regress/regress-167658.js#17
Updated•7 years ago
|
Attachment #8923436 -
Flags: review?(jwalden+bmo) → review+
Comment 115•7 years ago
|
||
Comment on attachment 8923443 [details] [diff] [review] bug1346234-part41-move-code-into-browser-iife.patch Review of attachment 8923443 [details] [diff] [review]: ----------------------------------------------------------------- rs=me I hope we can slim this down right quick -- it's gonna slow down shell tests as long as it's yuge like this. Still worth the tradeoff to have it all defined (with the occasional feature-testing guard) in one file rather than smooshed across two.
Attachment #8923443 -
Flags: review?(jwalden+bmo) → review+
Comment 116•7 years ago
|
||
Comment on attachment 8923452 [details] [diff] [review] bug1346234-part42-browser-iife-cleanup.patch Review of attachment 8923452 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/tests/browser.js @@ +226,1 @@ > var DocumentCreateElement = global.document.createElement; Seems nicer for these to be not global-qualified given the local above. @@ +240,5 @@ > + var TestCase = global.TestCase; > + > + var SpecialPowers = global.SpecialPowers; > + var SpecialPowersCu = SpecialPowers.Cu; > + var SpecialPowersForceGC = SpecialPowers.forceGC; Random thought, but we're probably past the point where any of this has value running in a browser/shell that doesn't support ES6 const (such runnability being the only reason this stuff didn't use newfangled things, for a long time), so might be worth making all these const in a separate patch. @@ +322,4 @@ > > window.onerror = function (msg, page, line, column, error) { > // Restore options in case a test case used this common variable name. > + global.options = options; A const on |options| declared earlier would also make this line a lot more confidence-engendering. @@ +331,1 @@ > } Mild preference for |=== undefined| (with such variable added earlier if no one's defined one yet). @@ +404,3 @@ > // Unset all options before running any test code, cf. the call to > // |optionsClear| in shell.js' set-up code. > for (var optionName of ["strict", "werror", "strict_mode"]) { Change this to an indexed loop for resilience against test changes? (Mostly just for consistency with the rest of the IIFE, since I guess if this runs, it would run before a test could muck with anything.) @@ +404,4 @@ > // Unset all options before running any test code, cf. the call to > // |optionsClear| in shell.js' set-up code. > for (var optionName of ["strict", "werror", "strict_mode"]) { > + if (!(optionName in SpecialPowersCu)) ObjectHasOwnProperty for the same reason? @@ +415,5 @@ > } > > // Hash containing the set options, initially empty because we just turned > // off all options. > options.currvalues = Object.create(null); ...okay, all these dependencies are starting to pile up some. How about a followup patch to make this whole function consistent with the IIFE? (Or to make it more resilient, since I see some parts of this function were changed for partial resilience.) @@ +501,5 @@ > + if (properties.gczeal) { > + gczeal(Number(properties.gczeal)); > + } > + > + document.write(`<title>${ properties.test }<\/title>`); Setting document.title might be more efficient than spinning up HTML parsing and all that, in followup fix material. (We can't have that much document.write left at this point -- would be great to kill it all.) @@ +524,5 @@ > > if (!moduleTest) { > // XXX bc - the first document.written script is ignored if the protocol > // is file:. insert an empty script tag, to work around it. > + document.write("<script></script>"); This cracktastic hack can probably be removed in a followup. @@ +592,5 @@ > appendScript(0); > } > } > > // Overrides the definition from shell.js If this is inside the IIFE, it's no longer accurate, right? @@ +621,4 @@ > window.opener.reportCallBack(window.opener.gWindow); > } > + > + setTimeout("window.opener.runNextTest()", 250); Would be nice in followup to just pass the function directly (and if the function's not |this|-ready for this, fixing it accordingly).
Attachment #8923452 -
Flags: review?(jwalden+bmo) → review+
Comment 117•7 years ago
|
||
Comment on attachment 8923460 [details] [diff] [review] bug1346234-part46-remove-global-BUGNUMBER.patch Review of attachment 8923460 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, we really probably should just kill these. ::: js/src/tests/ecma_3/Unicode/uc-003.js @@ +3,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +var BUGNUMBER = 23608; > +var BUGNUMBER = 23607; wat
Attachment #8923460 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 118•7 years ago
|
||
Update commit message from "r=Waldo" to "rs=Waldo".
Attachment #8923443 -
Attachment is obsolete: true
Attachment #8923803 -
Flags: review+
Assignee | ||
Comment 119•7 years ago
|
||
Updated per review comments, carrying r+.
Attachment #8923452 -
Attachment is obsolete: true
Attachment #8923805 -
Flags: review+
Assignee | ||
Comment 120•7 years ago
|
||
Needed to be rebased after changes in part 42.
Attachment #8923455 -
Attachment is obsolete: true
Attachment #8923806 -
Flags: review+
Assignee | ||
Comment 121•7 years ago
|
||
(In reply to Jeff Walden [:Waldo] (I'm baaaaaaack...) from comment #116) > @@ +322,4 @@ > > > > window.onerror = function (msg, page, line, column, error) { > > // Restore options in case a test case used this common variable name. > > + global.options = options; > > A const on |options| declared earlier would also make this line a lot more > confidence-engendering. The whole line goes away in part 43. > @@ +404,3 @@ > > // Unset all options before running any test code, cf. the call to > > // |optionsClear| in shell.js' set-up code. > > for (var optionName of ["strict", "werror", "strict_mode"]) { > > Change this to an indexed loop for resilience against test changes? (Mostly > just for consistency with the rest of the IIFE, since I guess if this runs, > it would run before a test could muck with anything.) Since this code is only run at start-up before any test code runs, I left it as is, because making the complete |jsTestDriverBrowserInit| function resilient against changes creates too much bloat (at least at the moment). > @@ +415,5 @@ > > } > > > > // Hash containing the set options, initially empty because we just turned > > // off all options. > > options.currvalues = Object.create(null); > > ...okay, all these dependencies are starting to pile up some. How about a > followup patch to make this whole function consistent with the IIFE? (Or to > make it more resilient, since I see some parts of this function were changed > for partial resilience.) Maybe at a later time, when the whole js-version business is no longer necessary. But for now I'd like to finally land these patches. > @@ +524,5 @@ > > > > if (!moduleTest) { > > // XXX bc - the first document.written script is ignored if the protocol > > // is file:. insert an empty script tag, to work around it. > > + document.write("<script></script>"); > > This cracktastic hack can probably be removed in a followup. bug 1345600. :-) > > @@ +592,5 @@ > > appendScript(0); > > } > > } > > > > // Overrides the definition from shell.js > > If this is inside the IIFE, it's no longer accurate, right? Is removed as part of patch 45. > > @@ +621,4 @@ > > window.opener.reportCallBack(window.opener.gWindow); > > } > > + > > + setTimeout("window.opener.runNextTest()", 250); > > Would be nice in followup to just pass the function directly (and if the > function's not |this|-ready for this, fixing it accordingly). TBH I'm not even sure this code is needed/used. For example |reportCallBack| is never defined in the tree <https://searchfox.org/mozilla-central/search?q=reportCallBack&case=true&path=> and when skimming over <https://searchfox.org/mozilla-central/search?q=%5CbrunNextTest%5Cb&case=true®exp=true&path=>, I also don't spot a global |runNextTest| function. Maybe that's more of the undocumented Spider hooks, which are probably no longer needed (comment #58)?
Assignee | ||
Comment 122•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b5d84cec547c3c22d3c05efe32302d5065d9fcc
Keywords: checkin-needed
Comment 123•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8206349e5a1b Part 1: Remove comments mentioning "gTestcases" so it's easier to grep for actual uses. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/0dc7bc8eb800 Part 2: Remove direct access to gTestcases in test cases. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/6624a29b53d7 Part 3: Make the test cases array a local variable in shell.js. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/3141bba57fba Part 4: Remove writeFormattedResult from browser.js because it's never called. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/fc9c354acc24 Part 5: Remove duplicate test environment type variables from shell.js. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/5fdea511af7b Part 6: Remove global exports for FAILED and PASSED variables in shell.js. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/91eacf211c0c Part 7: Inline writeTestCaseResult into its only caller. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/54a2f9570a14 Part 8: Stop modifying test case results after construction. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/987d09cdb807 Part 9: Remove getTestCaseResult() call in shell.js's test() function. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/83932c556491 Part 10: Negative tests don't need to set global EXPECTED variable. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/db3761c2d772 Part 11: Allow to set testcase reason property in TestCase constructor. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/e925ffdb1986 Part 12: Remove return value from reportCompare and reportMatch. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/9a2652788b67 Part 13: Remove call to testcase-dump method from browser.js. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/78685c13d309 Part 14: Remove non-whitespace characters from marked as fails whitespace tests. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/6104ff53a628 Part 15: Stop exporting toPrinted function from shell.js. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/a40509cce62b Part 16: Remove the global STATUS variable from shell.js. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/6fcdbaf35834 Part 17: Stop exporting currentFunc from shell.js. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/64496ef98f77 Part 18: Use template strings to concatenate error messages. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/64c0b36b2266 Part 19: Remove parentheses check from enterFunc and exitFunc. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/c21da031e455 Part 20: Remove Promise feature checks. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/9ed1c2835a8f Part 21: Remove workaround for a previously failing lexical environment test. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/6a65c1d64278 Part 22: Remove unnecessary calls to options() function from tests. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/2f8a9bf5bcbb Part 23: Remove unnecessary calls to startTest function from jstests. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/caabba869c64 Part 24: Remove bogus BUGNUMBER and startTest/printBugNumber calls. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/14a200037b6c Part 25: Replace remaining startTest calls with printBugNumber. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/cc87782e222a Part 26: Replace no longer used startTest function from jstests harness. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/3c53e8d662fb Part 27: Remove calls to enterFunc/exitFunc if test case has only a single test function. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/48a4bfc73523 Part 28: Remove enterFunc/exitFunc calls from single test case with valid usage. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/4818776866ce Part 29: Remove enterFunc/exitFunc from jstests harness. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/fbc2c062588a Part 30: Remove calls to quit() function from jstests. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb2db960816 Part 31: Move global property GLOBAL to sub-test directories. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/ede34275944a Part 32: Remove optionsPush and optionsPop from jstests harness. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/4a50245ed7ea Part 33: Remove optionsReset function from jstests harness. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/a9de147f0d6f Part 34: More options processing cleanup for jstests. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/ffe1c9acf901 Part 35: Remove dead code in jsTestDriverBrowserInit. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/561d56136cc4 Part 36: Remove unused "name" property from testcase objects. rs=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/3b96c0dd2a6c Part 37: Remove global property SECTION from shell.js. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/c3fa5f2e2245 Part 38: Remove global EXPECTED variable. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/a875e8d20dd1 Part 39: Remove global VERSION variable. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/7bfcf525baba Part 40: Make the set-up code logic more easy to understand. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/06bbd672d3fb Part 41: Move the remaining test harness code into the IIFE in browser.js. rs=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/3477b6b49e67 Part 42: Clean-up and strengthen test harness code in browser.js. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/6b6102b5d8bf Part 43: Remove test harness export for "optionsClear" functions. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/917e3724bded Part 44: Skip browser-only tests when running in the shell. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/0f94f91d14f4 Part 45: Remove jsTestDriverEnd from shell.js because it's only called in browser tests. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/350879c83bcd Part 46: Remove global BUGNUMBER variable from shell.js. r=Waldo
Keywords: checkin-needed
Comment 124•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8206349e5a1b https://hg.mozilla.org/mozilla-central/rev/0dc7bc8eb800 https://hg.mozilla.org/mozilla-central/rev/6624a29b53d7 https://hg.mozilla.org/mozilla-central/rev/3141bba57fba https://hg.mozilla.org/mozilla-central/rev/fc9c354acc24 https://hg.mozilla.org/mozilla-central/rev/5fdea511af7b https://hg.mozilla.org/mozilla-central/rev/91eacf211c0c https://hg.mozilla.org/mozilla-central/rev/54a2f9570a14 https://hg.mozilla.org/mozilla-central/rev/987d09cdb807 https://hg.mozilla.org/mozilla-central/rev/83932c556491 https://hg.mozilla.org/mozilla-central/rev/db3761c2d772 https://hg.mozilla.org/mozilla-central/rev/e925ffdb1986 https://hg.mozilla.org/mozilla-central/rev/9a2652788b67 https://hg.mozilla.org/mozilla-central/rev/78685c13d309 https://hg.mozilla.org/mozilla-central/rev/6104ff53a628 https://hg.mozilla.org/mozilla-central/rev/a40509cce62b https://hg.mozilla.org/mozilla-central/rev/6fcdbaf35834 https://hg.mozilla.org/mozilla-central/rev/64496ef98f77 https://hg.mozilla.org/mozilla-central/rev/64c0b36b2266 https://hg.mozilla.org/mozilla-central/rev/c21da031e455 https://hg.mozilla.org/mozilla-central/rev/9ed1c2835a8f https://hg.mozilla.org/mozilla-central/rev/6a65c1d64278 https://hg.mozilla.org/mozilla-central/rev/2f8a9bf5bcbb https://hg.mozilla.org/mozilla-central/rev/caabba869c64 https://hg.mozilla.org/mozilla-central/rev/14a200037b6c https://hg.mozilla.org/mozilla-central/rev/cc87782e222a https://hg.mozilla.org/mozilla-central/rev/3c53e8d662fb https://hg.mozilla.org/mozilla-central/rev/48a4bfc73523 https://hg.mozilla.org/mozilla-central/rev/4818776866ce https://hg.mozilla.org/mozilla-central/rev/fbc2c062588a https://hg.mozilla.org/mozilla-central/rev/2cb2db960816 https://hg.mozilla.org/mozilla-central/rev/ede34275944a https://hg.mozilla.org/mozilla-central/rev/4a50245ed7ea https://hg.mozilla.org/mozilla-central/rev/a9de147f0d6f https://hg.mozilla.org/mozilla-central/rev/ffe1c9acf901 https://hg.mozilla.org/mozilla-central/rev/561d56136cc4 https://hg.mozilla.org/mozilla-central/rev/3b96c0dd2a6c https://hg.mozilla.org/mozilla-central/rev/c3fa5f2e2245 https://hg.mozilla.org/mozilla-central/rev/a875e8d20dd1 https://hg.mozilla.org/mozilla-central/rev/7bfcf525baba https://hg.mozilla.org/mozilla-central/rev/06bbd672d3fb https://hg.mozilla.org/mozilla-central/rev/3477b6b49e67 https://hg.mozilla.org/mozilla-central/rev/6b6102b5d8bf https://hg.mozilla.org/mozilla-central/rev/917e3724bded https://hg.mozilla.org/mozilla-central/rev/0f94f91d14f4 https://hg.mozilla.org/mozilla-central/rev/350879c83bcd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•