1346234 - Remove more junk from jsreftest helpers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[André Bargull [:anba]]

... and further encapsulate internal test harness functions.

Comment 1

[André Bargull [:anba]]

Attachment: bug1346234-part1-gtestcases-comments.patch

Removes "gTestcases" from comments in preparation for parts 2-3.

Comment 2

[André Bargull [:anba]]

Attachment: bug1346234-part2-gtestcases-access.patch

Remove direct access to the global shell.js variables "gTestcases" and "gTc" from tests.

Comment 3

[André Bargull [:anba]]

Attachment: bug1346234-part3-gtestcases-encapsulated.patch

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.

Comment 4

[André Bargull [:anba]]

Attachment: bug1346234-part4-remove-writeformatted-from-browser.patch

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.

Comment 5

[André Bargull [:anba]]

Attachment: bug1346234-part5-remove-dupl-env-type-vars.patch

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?

Comment 6

[André Bargull [:anba]]

Attachment: bug1346234-part6-passed-failed-exports.patch

|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.

Comment 7

[André Bargull [:anba]]

Attachment: bug1346234-part7-inline-writetestcaseresult.patch

Inlines writeTestCaseResult and removes the try-catch workaround for js1.7

Comment 8

[André Bargull [:anba]]

Attachment: bug1346234-part8-no-modify-after-construction.patch

Avoid modifying test case result objects after construction, so we can later stop exposing the test case objects.

Comment 9

[André Bargull [:anba]]

Attachment: bug1346234-part9-second-gettestcase-result.patch

Removes the duplicate call to getTestCaseResult() to set the |.passed| property of test case objects. (And moves getTestCaseResult() closer to its only call site.)

Comment 10

[André Bargull [:anba]]

Attachment: bug1346234-part10-remove-global-expected-negative.patch

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").

Comment 11

[André Bargull [:anba]]

Attachment: bug1346234-part11-testcase-reason-argument.patch

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.

Comment 12

[André Bargull [:anba]]

Attachment: bug1346234-part12-return-val-report-functions.patch

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.).

Comment 13

[André Bargull [:anba]]

Attachment: bug1346234-part13-testcase-dump-from-browser.patch

TestCase.prototype.dump is a no-op in browser runs, so stop calling it in jsTestDriverEnd().

Comment 14

[André Bargull [:anba]]

Attachment: bug1346234-part14-fix-whitespace-fails-tests.patch

Just fix the test case instead of marking it as "|reftest| fails".

Comment 15

[André Bargull [:anba]]

Attachment: bug1346234-part15-stop-toprinted-export.patch

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.

Comment 16

[André Bargull [:anba]]

Attachment: bug1346234-part16-global-status.patch

Remove the global |STATUS| variable from shell.js

Comment 17

[André Bargull [:anba]]

Attachment: bug1346234-part17-currenfunc-export.patch

Tests don't call |currentFunc()| so we don't need to export it.

Comment 18

[André Bargull [:anba]]

Attachment: bug1346234-part18-templ-string-err-messages.patch

Use template strings for readability. And add a helper |newReportCompareTestCase| to reduce code duplication in |reportCompare| and |reportMatch|.

Comment 19

[André Bargull [:anba]]

Attachment: bug1346234-part19-enterexitfunc-paren-check.patch

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|.

Comment 20

[André Bargull [:anba]]

Attachment: bug1346234-part20-promise-feature-checks.patch

Remove feature guards for Promises because they're enabled by default.

Comment 21

[André Bargull [:anba]]

Attachment: bug1346234-part21-remove-lexevn-workaround.patch

Remove a no longer needed workaround for lexical environments.

Comment 22

[André Bargull [:anba]]

Attachment: bug1346234-part22-unnecessary-options-calls.patch

It's no longer necessary to call options('strict') or options('werror') to enable syntax errors for variables using keywords as names.

Comment 23

[André Bargull [:anba]]

Attachment: bug1346234-part23-unnecessary-starttest-calls.patch

Calling |startTest()| is a no-op when |BUGNUMBER| isn't set, so remove all |startTest()| calls when |BUGNUMBER| wasn't set earlier.

Comment 24

[André Bargull [:anba]]

Attachment: bug1346234-part24-bogus-bugnumber.patch

Remove uses of |BUGNUMBER| when no actual bug number is set.

Comment 25

[André Bargull [:anba]]

Attachment: bug1346234-part25-replace-starttest-with-printbugnumber.patch

Replace the remaining calls to |startTest()| with |printBugNumber(BUGNUMBER)|, because they're equivalent after part 23.

Comment 26

[André Bargull [:anba]]

Attachment: bug1346234-part26-remove-starttest.patch

|startTest| is no longer used, so let's remove it from shell.js

Comment 27

[André Bargull [:anba]]

Attachment: bug1346234-part27-enterexitfunc-single-testcase.patch

Remove calls to enter/exitFunc when there's only a single test case function. (In preparation for complete removal of enter/exitFunc.)

Comment 28

[André Bargull [:anba]]

Attachment: bug1346234-part28-remove-enterfunc-useful-test.patch

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.

Comment 29

[André Bargull [:anba]]

Attachment: bug1346234-part29-remove-enterexitfunc.patch

Actual removal of enter/exitFunc from shell.js

Comment 30

[André Bargull [:anba]]

Attachment: bug1346234-part30-remove-quit-calls.patch

Avoid calling |quit()| in tests because it doesn't do what you expect to do in the browser runner.

Comment 31

[André Bargull [:anba]]

Attachment: bug1346234-part31-move-global-GLOBAL-to-subtests.patch

Move the global |GLOBAL| variable to the tests which actually use it.

Comment 32

[André Bargull [:anba]]

Attachment: bug1346234-part32-remove-options-pushpop.patch

|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.

Comment 33

[André Bargull [:anba]]

Attachment: bug1346234-part33-remove-options-reset.patch

|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!

Comment 34

[André Bargull [:anba]]

Attachment: bug1346234-part34-more-options-cleanup.patch

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).

Comment 35

[André Bargull [:anba]]

Attachment: bug1346234-part35-remove-unused-globals.patch

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.

Comment 36

[André Bargull [:anba]]

Attachment: bug1346234-part36-remove-testcase-name-property.patch

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.

Comment 37

[André Bargull [:anba]]

Attachment: bug1346234-part37-remove-section-global.patch

Removes the global |SECTION| variable from shell.js, because it's no longer used.

Comment 38

[André Bargull [:anba]]

Attachment: bug1346234-part18-templ-string-err-messages.patch

Update part 18 to apply cleanly on inbound.

Comment 39

[André Bargull [:anba]]

Attachment: bug1346234-part24-bogus-bugnumber.patch

Update part 24 to apply cleanly on inbound.

Comment 40

[André Bargull [:anba]]

Attachment: bug1346234-part27-enterexitfunc-single-testcase.patch

Update part 27 to apply cleanly on inbound.

Comment 41

[André Bargull [:anba]]

Attachment: bug1346234-part30-remove-quit-calls.patch

Update part 30 to apply cleanly on inbound.

Comment 42

[André Bargull [:anba]]

Attachment: bug1346234-part36-remove-testcase-name-property.patch

Update part 36 to apply cleanly on inbound.

Comment 43

[André Bargull [:anba]]

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.

Comment 44

[André Bargull [:anba]]

Comment on attachment 8845974 [details] [diff] [review]
bug1346234-part2-gtestcases-access.patch

See comment #43 about the "addTestCase" pattern.

Comment 45

[André Bargull [:anba]]

Comment on attachment 8846033 [details] [diff] [review]
bug1346234-part19-enterexitfunc-paren-check.patch

enterFunc and exitFunc will actually be removed later (see comment #43)!

Comment 46

[Jeff Walden [:Waldo]]

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.

Comment 47

[Jeff Walden [:Waldo]]

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.

Comment 48

[Jeff Walden [:Waldo]]

Comment on attachment 8846027 [details] [diff] [review]
bug1346234-part14-fix-whitespace-fails-tests.patch

Review of attachment 8846027 [details] [diff] [review]:
-----------------------------------------------------------------

lol

Comment 49

[Jeff Walden [:Waldo]]

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.

Comment 50

[Jeff Walden [:Waldo]]

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/

Comment 51

[Jeff Walden [:Waldo]]

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.

Comment 52

[Jeff Walden [:Waldo]]

(I did look at the shell.js bit of it specifically, tho.)

Comment 53

[Steve Fink [:sfink] [:s:]]

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?

Comment 54

[Steve Fink [:sfink] [:s:]]

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?)

Comment 55

[Steve Fink [:sfink] [:s:]]

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 !== "".

Comment 56

[Steve Fink [:sfink] [:s:]]

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?

Comment 57

[André Bargull [:anba]]

(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.

Comment 58

[André Bargull [:anba]]

Oh, it looks like the hooks for Spider are probably no longer necessary (bug 1396088).

Comment 59

[André Bargull [:anba]]

(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.

Comment 60

[André Bargull [:anba]]

(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!

Comment 61

[André Bargull [:anba]]

Attachment: bug1346234-part1-gtestcases-comments.patch

Comment 62

[André Bargull [:anba]]

Attachment: bug1346234-part2-gtestcases-access.patch

Comment 63

[André Bargull [:anba]]

Attachment: bug1346234-part3-gtestcases-encapsulated.patch

Comment 64

[André Bargull [:anba]]

Attachment: bug1346234-part4-remove-writeformatted-from-browser.patch

Comment 65

[André Bargull [:anba]]

Attachment: bug1346234-part5-remove-dupl-env-type-vars.patch

Comment 66

[André Bargull [:anba]]

Attachment: bug1346234-part6-passed-failed-exports.patch

Comment 67

[André Bargull [:anba]]

Attachment: bug1346234-part7-inline-writetestcaseresult.patch

Comment 68

[André Bargull [:anba]]

Attachment: bug1346234-part8-no-modify-after-construction.patch

Comment 69

[André Bargull [:anba]]

Attachment: bug1346234-part9-second-gettestcase-result.patch

Comment 70

[André Bargull [:anba]]

Attachment: bug1346234-part10-remove-global-expected-negative.patch

Comment 71

[André Bargull [:anba]]

Attachment: bug1346234-part11-testcase-reason-argument.patch

Comment 72

[André Bargull [:anba]]

Attachment: bug1346234-part12-return-val-report-functions.patch

Comment 73

[André Bargull [:anba]]

Attachment: bug1346234-part13-testcase-dump-from-browser.patch

Comment 74

[André Bargull [:anba]]

Attachment: bug1346234-part14-fix-whitespace-fails-tests.patch

Comment 75

[André Bargull [:anba]]

Attachment: bug1346234-part15-stop-toprinted-export.patch

Comment 76

[André Bargull [:anba]]

Attachment: bug1346234-part16-global-status.patch

Comment 77

[André Bargull [:anba]]

Attachment: bug1346234-part17-currenfunc-export.patch

Comment 78

[André Bargull [:anba]]

Attachment: bug1346234-part18-templ-string-err-messages.patch

Renamed harness function to "reportTestCaseResult" per review comments.

Comment 79

[André Bargull [:anba]]

Attachment: bug1346234-part19-enterexitfunc-paren-check.patch

Comment 80

[André Bargull [:anba]]

Attachment: bug1346234-part20-promise-feature-checks.patch

Comment 81

[André Bargull [:anba]]

Attachment: bug1346234-part21-remove-lexevn-workaround.patch

Comment 82

[André Bargull [:anba]]

Attachment: bug1346234-part22-unnecessary-options-calls.patch

Comment 83

[André Bargull [:anba]]

Attachment: bug1346234-part23-unnecessary-starttest-calls.patch

Comment 84

[André Bargull [:anba]]

Attachment: bug1346234-part24-bogus-bugnumber.patch

Comment 85

[André Bargull [:anba]]

Attachment: bug1346234-part25-replace-starttest-with-printbugnumber.patch

Comment 86

[André Bargull [:anba]]

Attachment: bug1346234-part26-remove-starttest.patch

Comment 87

[André Bargull [:anba]]

Attachment: bug1346234-part27-enterexitfunc-single-testcase.patch

Comment 88

[André Bargull [:anba]]

Attachment: bug1346234-part28-remove-enterfunc-useful-test.patch

Comment 89

[André Bargull [:anba]]

Attachment: bug1346234-part29-remove-enterexitfunc.patch

Comment 90

[André Bargull [:anba]]

Attachment: bug1346234-part30-remove-quit-calls.patch

Comment 91

[André Bargull [:anba]]

Attachment: bug1346234-part31-move-global-GLOBAL-to-subtests.patch

Comment 92

[André Bargull [:anba]]

Attachment: bug1346234-part32-remove-options-pushpop.patch

Comment 93

[André Bargull [:anba]]

Attachment: bug1346234-part33-remove-options-reset.patch

Comment 94

[André Bargull [:anba]]

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").

Comment 95

[André Bargull [:anba]]

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.

Comment 96

[André Bargull [:anba]]

Attachment: bug1346234-part34-more-options-cleanup.patch

More clean-ups for this part of the test framework are in part 40 and 43.

Comment 97

[André Bargull [:anba]]

Attachment: bug1346234-part35-remove-unused-globals.patch

Comment 98

[André Bargull [:anba]]

Attachment: bug1346234-part36-remove-testcase-name-property.patch

Comment 99

[André Bargull [:anba]]

Attachment: bug1346234-part37-remove-section-global.patch

Comment 100

[André Bargull [:anba]]

Updated parts 1-37 to include the reviewer name in the commit message, carrying r+ where applicable.

Comment 101

[André Bargull [:anba]]

Attachment: bug1346234-part38-remove-global-EXPECTED.patch

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.

Comment 102

[André Bargull [:anba]]

Attachment: bug1346234-part39-remove-global-VERSION.patch

Removes the global |VERSION| variable from all tests and the test harness (the variable is never read/used).

Comment 103

[André Bargull [:anba]]

Attachment: bug1346234-part40-test-framework-setup-cleanup.patch

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.

Comment 104

[André Bargull [:anba]]

Attachment: bug1346234-part41-move-code-into-browser-iife.patch

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).

Comment 105

[André Bargull [:anba]]

Attachment: bug1346234-part42-browser-iife-cleanup.patch

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...

Comment 106

[André Bargull [:anba]]

Attachment: bug1346234-part43-remove-optionsClear-export.patch

- 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|.

Comment 107

[André Bargull [:anba]]

Attachment: bug1346234-part44-skip-browseronly-tests-in-shell.patch

- 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.

Comment 108

[André Bargull [:anba]]

Attachment: bug1346234-part45-remove-unused-jsTestDriverEnd-from-shell.patch

- 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. :-)

Comment 109

[André Bargull [:anba]]

Attachment: bug1346234-part46-remove-global-BUGNUMBER.patch

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.)

Comment 110

[André Bargull [:anba]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1eb5036d10f14189ce389b2fc36c0bf1d22c2067

Comment 111

[Steve Fink [:sfink] [:s:]]

(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.

Comment 112

[Steve Fink [:sfink] [:s:]]

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.

Comment 113

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8923456 [details] [diff] [review]
bug1346234-part44-skip-browseronly-tests-in-shell.patch

Review of attachment 8923456 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

Comment 114

[André Bargull [:anba]]

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

Comment 115

[Jeff Walden [:Waldo]]

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.

Comment 116

[Jeff Walden [:Waldo]]

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).

Comment 117

[Jeff Walden [:Waldo]]

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

Comment 118

[André Bargull [:anba]]

Attachment: bug1346234-part41-move-code-into-browser-iife.patch

Update commit message from "r=Waldo" to "rs=Waldo".

Comment 119

[André Bargull [:anba]]

Attachment: bug1346234-part42-browser-iife-cleanup.patch

Updated per review comments, carrying r+.

Comment 120

[André Bargull [:anba]]

Attachment: bug1346234-part43-remove-optionsClear-export.patch

Needed to be rebased after changes in part 42.

Comment 121

[André Bargull [:anba]]

(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&regexp=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)?

Comment 122

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b5d84cec547c3c22d3c05efe32302d5065d9fcc

Comment 123

[Pulsebot]

Pushed by [e-mail]:
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

Comment 124

[Natalia Csoregi [:nataliaCs]]

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