Remove more junk from jsreftest helpers

RESOLVED FIXED in Firefox 58

Status

()

Core
JavaScript Engine
P3
normal
RESOLVED FIXED
a year ago
6 months ago

People

(Reporter: anba, Assigned: anba)

Tracking

({triage-deferred})

Trunk
mozilla58
triage-deferred
Points:
---

Firefox Tracking Flags

(firefox55 affected, firefox58 fixed)

Details

Attachments

(46 attachments, 45 obsolete attachments)

7.79 KB, patch
anba
: review+
Details | Diff | Splinter Review
28.23 KB, patch
anba
: review+
Details | Diff | Splinter Review
5.32 KB, patch
anba
: review+
Details | Diff | Splinter Review
5.51 KB, patch
anba
: review+
Details | Diff | Splinter Review
4.78 KB, patch
anba
: review+
Details | Diff | Splinter Review
1.73 KB, patch
anba
: review+
Details | Diff | Splinter Review
2.78 KB, patch
anba
: review+
Details | Diff | Splinter Review
14.13 KB, patch
anba
: review+
Details | Diff | Splinter Review
3.92 KB, patch
anba
: review+
Details | Diff | Splinter Review
58.28 KB, patch
anba
: review+
Details | Diff | Splinter Review
6.41 KB, patch
anba
: review+
Details | Diff | Splinter Review
5.40 KB, patch
anba
: review+
Details | Diff | Splinter Review
1.04 KB, patch
anba
: review+
Details | Diff | Splinter Review
4.33 KB, patch
anba
: review+
Details | Diff | Splinter Review
2.65 KB, patch
anba
: review+
Details | Diff | Splinter Review
1.25 KB, patch
anba
: review+
Details | Diff | Splinter Review
896 bytes, patch
anba
: review+
Details | Diff | Splinter Review
8.04 KB, patch
anba
: review+
Details | Diff | Splinter Review
4.22 KB, patch
anba
: review+
Details | Diff | Splinter Review
9.45 KB, patch
anba
: review+
Details | Diff | Splinter Review
1.32 KB, patch
anba
: review+
Details | Diff | Splinter Review
8.78 KB, patch
anba
: review+
Details | Diff | Splinter Review
506.70 KB, patch
anba
: review+
Details | Diff | Splinter Review
19.04 KB, patch
anba
: review+
Details | Diff | Splinter Review
55.80 KB, patch
anba
: review+
Details | Diff | Splinter Review
1.17 KB, patch
anba
: review+
Details | Diff | Splinter Review
702.86 KB, patch
anba
: review+
Details | Diff | Splinter Review
5.28 KB, patch
anba
: review+
Details | Diff | Splinter Review
5.32 KB, patch
anba
: review+
Details | Diff | Splinter Review
10.85 KB, patch
anba
: review+
Details | Diff | Splinter Review
2.68 KB, patch
anba
: review+
Details | Diff | Splinter Review
2.86 KB, patch
anba
: review+
Details | Diff | Splinter Review
3.70 KB, patch
anba
: review+
Details | Diff | Splinter Review
5.09 KB, patch
sfink
: review+
Details | Diff | Splinter Review
904 bytes, patch
anba
: review+
Details | Diff | Splinter Review
1.86 MB, patch
anba
: review+
Details | Diff | Splinter Review
1.46 KB, patch
anba
: review+
Details | Diff | Splinter Review
3.00 KB, patch
sfink
: review+
Details | Diff | Splinter Review
593.98 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
3.09 KB, patch
sfink
: review+
Details | Diff | Splinter Review
27.39 KB, patch
sfink
: review+
Details | Diff | Splinter Review
4.67 KB, patch
sfink
: review+
Details | Diff | Splinter Review
14.15 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
24.68 KB, patch
anba
: review+
Details | Diff | Splinter Review
22.81 KB, patch
anba
: review+
Details | Diff | Splinter Review
9.51 KB, patch
anba
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
... and further encapsulate internal test harness functions.
(Assignee)

Comment 1

a year ago
Created attachment 8845973 [details] [diff] [review]
bug1346234-part1-gtestcases-comments.patch

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

Comment 2

a year ago
Created attachment 8845974 [details] [diff] [review]
bug1346234-part2-gtestcases-access.patch

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

Comment 3

a year ago
Created attachment 8845994 [details] [diff] [review]
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.
(Assignee)

Comment 4

a year ago
Created attachment 8845997 [details] [diff] [review]
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.
(Assignee)

Comment 5

a year ago
Created attachment 8846000 [details] [diff] [review]
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?
(Assignee)

Comment 6

a year ago
Created attachment 8846004 [details] [diff] [review]
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.
(Assignee)

Comment 7

a year ago
Created attachment 8846005 [details] [diff] [review]
bug1346234-part7-inline-writetestcaseresult.patch

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

Comment 8

a year ago
Created attachment 8846006 [details] [diff] [review]
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.
(Assignee)

Comment 9

a year ago
Created attachment 8846007 [details] [diff] [review]
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.)
(Assignee)

Comment 10

a year ago
Created attachment 8846008 [details] [diff] [review]
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").
(Assignee)

Comment 11

a year ago
Created attachment 8846011 [details] [diff] [review]
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.
(Assignee)

Comment 12

a year ago
Created attachment 8846025 [details] [diff] [review]
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.).
(Assignee)

Comment 13

a year ago
Created attachment 8846026 [details] [diff] [review]
bug1346234-part13-testcase-dump-from-browser.patch

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

Comment 14

a year ago
Created attachment 8846027 [details] [diff] [review]
bug1346234-part14-fix-whitespace-fails-tests.patch

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

Comment 15

a year ago
Created attachment 8846029 [details] [diff] [review]
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.
(Assignee)

Comment 16

a year ago
Created attachment 8846030 [details] [diff] [review]
bug1346234-part16-global-status.patch

Remove the global |STATUS| variable from shell.js
(Assignee)

Comment 17

a year ago
Created attachment 8846031 [details] [diff] [review]
bug1346234-part17-currenfunc-export.patch

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

Comment 18

a year ago
Created attachment 8846032 [details] [diff] [review]
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|.
(Assignee)

Comment 19

a year ago
Created attachment 8846033 [details] [diff] [review]
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|.
(Assignee)

Comment 20

a year ago
Created attachment 8846034 [details] [diff] [review]
bug1346234-part20-promise-feature-checks.patch

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

Comment 21

a year ago
Created attachment 8846035 [details] [diff] [review]
bug1346234-part21-remove-lexevn-workaround.patch

Remove a no longer needed workaround for lexical environments.
(Assignee)

Comment 22

a year ago
Created attachment 8846036 [details] [diff] [review]
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.
(Assignee)

Comment 23

a year ago
Created attachment 8846037 [details] [diff] [review]
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.
(Assignee)

Comment 24

a year ago
Created attachment 8846038 [details] [diff] [review]
bug1346234-part24-bogus-bugnumber.patch

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

Comment 25

a year ago
Created attachment 8846039 [details] [diff] [review]
bug1346234-part25-replace-starttest-with-printbugnumber.patch

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

Comment 26

a year ago
Created attachment 8846040 [details] [diff] [review]
bug1346234-part26-remove-starttest.patch

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

Comment 27

a year ago
Created attachment 8846041 [details] [diff] [review]
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.)
(Assignee)

Comment 28

a year ago
Created attachment 8846043 [details] [diff] [review]
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.
(Assignee)

Comment 29

a year ago
Created attachment 8846044 [details] [diff] [review]
bug1346234-part29-remove-enterexitfunc.patch

Actual removal of enter/exitFunc from shell.js
(Assignee)

Comment 30

a year ago
Created attachment 8846045 [details] [diff] [review]
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.
(Assignee)

Comment 31

a year ago
Created attachment 8846046 [details] [diff] [review]
bug1346234-part31-move-global-GLOBAL-to-subtests.patch

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

Comment 32

a year ago
Created attachment 8846047 [details] [diff] [review]
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.
(Assignee)

Comment 33

a year ago
Created attachment 8846049 [details] [diff] [review]
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!
(Assignee)

Comment 34

a year ago
Created attachment 8846050 [details] [diff] [review]
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).
(Assignee)

Comment 35

a year ago
Created attachment 8846051 [details] [diff] [review]
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.
(Assignee)

Comment 36

a year ago
Created attachment 8846052 [details] [diff] [review]
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.
(Assignee)

Comment 37

a year ago
Created attachment 8846053 [details] [diff] [review]
bug1346234-part37-remove-section-global.patch

Removes the global |SECTION| variable from shell.js, because it's no longer used.
Keywords: triage-deferred
Priority: -- → P3
(Assignee)

Comment 38

6 months ago
Created attachment 8921974 [details] [diff] [review]
bug1346234-part18-templ-string-err-messages.patch

Update part 18 to apply cleanly on inbound.
Attachment #8846032 - Attachment is obsolete: true
(Assignee)

Comment 39

6 months ago
Created attachment 8921975 [details] [diff] [review]
bug1346234-part24-bogus-bugnumber.patch

Update part 24 to apply cleanly on inbound.
Attachment #8846038 - Attachment is obsolete: true
(Assignee)

Comment 40

6 months ago
Created attachment 8921976 [details] [diff] [review]
bug1346234-part27-enterexitfunc-single-testcase.patch

Update part 27 to apply cleanly on inbound.
Attachment #8846041 - Attachment is obsolete: true
(Assignee)

Comment 41

6 months ago
Created attachment 8921977 [details] [diff] [review]
bug1346234-part30-remove-quit-calls.patch

Update part 30 to apply cleanly on inbound.
Attachment #8846045 - Attachment is obsolete: true
(Assignee)

Comment 42

6 months ago
Created attachment 8921978 [details] [diff] [review]
bug1346234-part36-remove-testcase-name-property.patch

Update part 36 to apply cleanly on inbound.
Attachment #8846052 - Attachment is obsolete: true
(Assignee)

Comment 43

6 months 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

6 months ago
Attachment #8845973 - Flags: review?(sphink)
(Assignee)

Comment 44

6 months 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

6 months ago
Attachment #8845994 - Flags: review?(sphink)
(Assignee)

Updated

6 months ago
Attachment #8845997 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 months ago
Attachment #8846000 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 months ago
Attachment #8846004 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 months ago
Attachment #8846005 - Flags: review?(sphink)
(Assignee)

Updated

6 months ago
Attachment #8846006 - Flags: review?(sphink)
(Assignee)

Updated

6 months ago
Attachment #8846007 - Flags: review?(sphink)
(Assignee)

Updated

6 months ago
Attachment #8846008 - Flags: review?(sphink)
(Assignee)

Updated

6 months ago
Attachment #8846011 - Flags: review?(sphink)
(Assignee)

Updated

6 months ago
Attachment #8846025 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 months ago
Attachment #8846026 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 months ago
Attachment #8846027 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 months ago
Attachment #8846029 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 months ago
Attachment #8846030 - Flags: review?(sphink)
(Assignee)

Updated

6 months ago
Attachment #8846031 - Flags: review?(sphink)
(Assignee)

Updated

6 months ago
Attachment #8921974 - Flags: review?(sphink)
(Assignee)

Comment 45

6 months 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

6 months ago
Attachment #8846034 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 months ago
Attachment #8846035 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 months ago
Attachment #8846036 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 months ago
Attachment #8846037 - Flags: review?(sphink)
(Assignee)

Updated

6 months ago
Attachment #8921975 - Flags: review?(sphink)
(Assignee)

Updated

6 months ago
Attachment #8846039 - Flags: review?(sphink)
(Assignee)

Updated

6 months ago
Attachment #8846040 - Flags: review?(sphink)
(Assignee)

Updated

6 months ago
Attachment #8921976 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 months ago
Attachment #8846043 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 months ago
Attachment #8846044 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 months ago
Attachment #8921977 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 months ago
Attachment #8846046 - Flags: review?(sphink)
(Assignee)

Updated

6 months ago
Attachment #8846047 - Flags: review?(sphink)
(Assignee)

Updated

6 months ago
Attachment #8846049 - Flags: review?(sphink)
(Assignee)

Updated

6 months ago
Attachment #8846050 - Flags: review?(sphink)
(Assignee)

Updated

6 months ago
Attachment #8846051 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 months ago
Attachment #8921978 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 months ago
Attachment #8846053 - Flags: review?(jwalden+bmo)

Updated

6 months ago
Attachment #8845997 - Flags: review?(jwalden+bmo) → review+

Comment 46

6 months 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

6 months ago
Attachment #8846004 - Flags: review?(jwalden+bmo) → review+

Comment 47

6 months 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

6 months ago
Attachment #8846026 - Flags: review?(jwalden+bmo) → review+

Comment 48

6 months 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

6 months ago
Attachment #8846029 - Flags: review?(jwalden+bmo) → review+

Updated

6 months ago
Attachment #8846033 - Flags: review?(jwalden+bmo) → review+

Updated

6 months ago
Attachment #8846034 - Flags: review?(jwalden+bmo) → review+

Updated

6 months ago
Attachment #8846035 - Flags: review?(jwalden+bmo) → review+

Comment 49

6 months 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

6 months ago
Attachment #8921976 - Flags: review?(jwalden+bmo) → review+

Updated

6 months ago
Attachment #8846043 - Flags: review?(jwalden+bmo) → review+

Updated

6 months ago
Attachment #8846044 - Flags: review?(jwalden+bmo) → review+

Comment 50

6 months 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

6 months ago
Attachment #8846051 - Flags: review?(jwalden+bmo) → review+

Comment 51

6 months 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

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

Updated

6 months ago
Attachment #8846053 - Flags: review?(jwalden+bmo) → review+
Attachment #8845973 - Flags: review?(sphink) → review+
Attachment #8845974 - Flags: review?(sphink) → review+
Attachment #8845994 - Flags: review?(sphink) → review+
Attachment #8846005 - Flags: review?(sphink) → review+
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+
Attachment #8846007 - Flags: review?(sphink) → review+
Attachment #8846008 - Flags: review?(sphink) → review+
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+
Attachment #8846030 - Flags: review?(sphink) → review+
Attachment #8846031 - Flags: review?(sphink) → review+
Attachment #8846037 - Flags: review?(sphink) → review+
Attachment #8846039 - Flags: review?(sphink) → review+
Attachment #8846040 - Flags: review?(sphink) → review+
Attachment #8846046 - Flags: review?(sphink) → review+
Attachment #8846047 - Flags: review?(sphink) → review+
Attachment #8846049 - Flags: review?(sphink) → review+
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 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+
Attachment #8921975 - Flags: review?(sphink) → review+
(Assignee)

Comment 57

6 months 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

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

Comment 59

6 months 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

6 months 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

6 months ago
Created attachment 8923384 [details] [diff] [review]
bug1346234-part1-gtestcases-comments.patch
Attachment #8845973 - Attachment is obsolete: true
Attachment #8923384 - Flags: review+
(Assignee)

Comment 62

6 months ago
Created attachment 8923387 [details] [diff] [review]
bug1346234-part2-gtestcases-access.patch
Attachment #8845974 - Attachment is obsolete: true
Attachment #8923387 - Flags: review+
(Assignee)

Comment 63

6 months ago
Created attachment 8923388 [details] [diff] [review]
bug1346234-part3-gtestcases-encapsulated.patch
Attachment #8845994 - Attachment is obsolete: true
Attachment #8923388 - Flags: review+
(Assignee)

Comment 64

6 months ago
Created attachment 8923389 [details] [diff] [review]
bug1346234-part4-remove-writeformatted-from-browser.patch
Attachment #8845997 - Attachment is obsolete: true
Attachment #8923389 - Flags: review+
(Assignee)

Comment 65

6 months ago
Created attachment 8923390 [details] [diff] [review]
bug1346234-part5-remove-dupl-env-type-vars.patch
Attachment #8846000 - Attachment is obsolete: true
Attachment #8923390 - Flags: review+
(Assignee)

Comment 66

6 months ago
Created attachment 8923391 [details] [diff] [review]
bug1346234-part6-passed-failed-exports.patch
Attachment #8846004 - Attachment is obsolete: true
Attachment #8923391 - Flags: review+
(Assignee)

Comment 67

6 months ago
Created attachment 8923392 [details] [diff] [review]
bug1346234-part7-inline-writetestcaseresult.patch
Attachment #8923392 - Flags: review+
(Assignee)

Updated

6 months ago
Attachment #8846005 - Attachment is obsolete: true
(Assignee)

Comment 68

6 months ago
Created attachment 8923397 [details] [diff] [review]
bug1346234-part8-no-modify-after-construction.patch
Attachment #8846006 - Attachment is obsolete: true
Attachment #8923397 - Flags: review+
(Assignee)

Comment 69

6 months ago
Created attachment 8923398 [details] [diff] [review]
bug1346234-part9-second-gettestcase-result.patch
Attachment #8846007 - Attachment is obsolete: true
Attachment #8923398 - Flags: review+
(Assignee)

Comment 70

6 months ago
Created attachment 8923399 [details] [diff] [review]
bug1346234-part10-remove-global-expected-negative.patch
Attachment #8846008 - Attachment is obsolete: true
Attachment #8923399 - Flags: review+
(Assignee)

Comment 71

6 months ago
Created attachment 8923400 [details] [diff] [review]
bug1346234-part11-testcase-reason-argument.patch
Attachment #8846011 - Attachment is obsolete: true
Attachment #8923400 - Flags: review+
(Assignee)

Comment 72

6 months ago
Created attachment 8923401 [details] [diff] [review]
bug1346234-part12-return-val-report-functions.patch
Attachment #8846025 - Attachment is obsolete: true
Attachment #8923401 - Flags: review+
(Assignee)

Comment 73

6 months ago
Created attachment 8923402 [details] [diff] [review]
bug1346234-part13-testcase-dump-from-browser.patch
Attachment #8846026 - Attachment is obsolete: true
Attachment #8923402 - Flags: review+
(Assignee)

Comment 74

6 months ago
Created attachment 8923403 [details] [diff] [review]
bug1346234-part14-fix-whitespace-fails-tests.patch
Attachment #8846027 - Attachment is obsolete: true
Attachment #8923403 - Flags: review+
(Assignee)

Comment 75

6 months ago
Created attachment 8923404 [details] [diff] [review]
bug1346234-part15-stop-toprinted-export.patch
Attachment #8846029 - Attachment is obsolete: true
Attachment #8923404 - Flags: review+
(Assignee)

Comment 76

6 months ago
Created attachment 8923405 [details] [diff] [review]
bug1346234-part16-global-status.patch
Attachment #8846030 - Attachment is obsolete: true
Attachment #8923405 - Flags: review+
(Assignee)

Comment 77

6 months ago
Created attachment 8923406 [details] [diff] [review]
bug1346234-part17-currenfunc-export.patch
Attachment #8846031 - Attachment is obsolete: true
Attachment #8923406 - Flags: review+
(Assignee)

Comment 78

6 months ago
Created attachment 8923407 [details] [diff] [review]
bug1346234-part18-templ-string-err-messages.patch

Renamed harness function to "reportTestCaseResult" per review comments.
Attachment #8921974 - Attachment is obsolete: true
Attachment #8923407 - Flags: review+
(Assignee)

Comment 79

6 months ago
Created attachment 8923408 [details] [diff] [review]
bug1346234-part19-enterexitfunc-paren-check.patch
Attachment #8846033 - Attachment is obsolete: true
Attachment #8923408 - Flags: review+
(Assignee)

Comment 80

6 months ago
Created attachment 8923409 [details] [diff] [review]
bug1346234-part20-promise-feature-checks.patch
Attachment #8846034 - Attachment is obsolete: true
Attachment #8923409 - Flags: review+
(Assignee)

Comment 81

6 months ago
Created attachment 8923410 [details] [diff] [review]
bug1346234-part21-remove-lexevn-workaround.patch
Attachment #8846035 - Attachment is obsolete: true
Attachment #8923410 - Flags: review+
(Assignee)

Comment 82

6 months ago
Created attachment 8923411 [details] [diff] [review]
bug1346234-part22-unnecessary-options-calls.patch
Attachment #8846036 - Attachment is obsolete: true
Attachment #8923411 - Flags: review+
(Assignee)

Comment 83

6 months ago
Created attachment 8923412 [details] [diff] [review]
bug1346234-part23-unnecessary-starttest-calls.patch
Attachment #8846037 - Attachment is obsolete: true
Attachment #8923412 - Flags: review+
(Assignee)

Comment 84

6 months ago
Created attachment 8923413 [details] [diff] [review]
bug1346234-part24-bogus-bugnumber.patch
Attachment #8921975 - Attachment is obsolete: true
Attachment #8923413 - Flags: review+
(Assignee)

Comment 85

6 months ago
Created attachment 8923414 [details] [diff] [review]
bug1346234-part25-replace-starttest-with-printbugnumber.patch
Attachment #8846039 - Attachment is obsolete: true
Attachment #8923414 - Flags: review+
(Assignee)

Comment 86

6 months ago
Created attachment 8923415 [details] [diff] [review]
bug1346234-part26-remove-starttest.patch
Attachment #8846040 - Attachment is obsolete: true
Attachment #8923415 - Flags: review+
(Assignee)

Comment 87

6 months ago
Created attachment 8923416 [details] [diff] [review]
bug1346234-part27-enterexitfunc-single-testcase.patch
Attachment #8921976 - Attachment is obsolete: true
Attachment #8923416 - Flags: review+
(Assignee)

Comment 88

6 months ago
Created attachment 8923417 [details] [diff] [review]
bug1346234-part28-remove-enterfunc-useful-test.patch
Attachment #8846043 - Attachment is obsolete: true
Attachment #8923417 - Flags: review+
(Assignee)

Comment 89

6 months ago
Created attachment 8923418 [details] [diff] [review]
bug1346234-part29-remove-enterexitfunc.patch
Attachment #8846044 - Attachment is obsolete: true
Attachment #8923418 - Flags: review+
(Assignee)

Comment 90

6 months ago
Created attachment 8923419 [details] [diff] [review]
bug1346234-part30-remove-quit-calls.patch
Attachment #8921977 - Attachment is obsolete: true
Attachment #8923419 - Flags: review+
(Assignee)

Comment 91

6 months ago
Created attachment 8923420 [details] [diff] [review]
bug1346234-part31-move-global-GLOBAL-to-subtests.patch
Attachment #8846046 - Attachment is obsolete: true
Attachment #8923420 - Flags: review+
(Assignee)

Comment 92

6 months ago
Created attachment 8923421 [details] [diff] [review]
bug1346234-part32-remove-options-pushpop.patch
Attachment #8846047 - Attachment is obsolete: true
Attachment #8923421 - Flags: review+
(Assignee)

Comment 93

6 months ago
Created attachment 8923422 [details] [diff] [review]
bug1346234-part33-remove-options-reset.patch
Attachment #8846049 - Attachment is obsolete: true
Attachment #8923422 - Flags: review+
(Assignee)

Comment 94

6 months 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

6 months 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

6 months ago
Created attachment 8923424 [details] [diff] [review]
bug1346234-part34-more-options-cleanup.patch

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

6 months ago
Created attachment 8923425 [details] [diff] [review]
bug1346234-part35-remove-unused-globals.patch
Attachment #8846051 - Attachment is obsolete: true
Attachment #8923425 - Flags: review+
(Assignee)

Comment 98

6 months ago
Created attachment 8923427 [details] [diff] [review]
bug1346234-part36-remove-testcase-name-property.patch
Attachment #8921978 - Attachment is obsolete: true
Attachment #8923427 - Flags: review+
(Assignee)

Comment 99

6 months ago
Created attachment 8923428 [details] [diff] [review]
bug1346234-part37-remove-section-global.patch
Attachment #8846053 - Attachment is obsolete: true
Attachment #8923428 - Flags: review+
(Assignee)

Comment 100

6 months ago
Updated parts 1-37 to include the reviewer name in the commit message, carrying r+ where applicable.
(Assignee)

Comment 101

6 months ago
Created attachment 8923435 [details] [diff] [review]
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.
Attachment #8923435 - Flags: review?(sphink)
(Assignee)

Comment 102

6 months ago
Created attachment 8923436 [details] [diff] [review]
bug1346234-part39-remove-global-VERSION.patch

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

6 months ago
Created attachment 8923439 [details] [diff] [review]
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.
Attachment #8923439 - Flags: review?(sphink)
(Assignee)

Comment 104

6 months ago
Created attachment 8923443 [details] [diff] [review]
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).
Attachment #8923443 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 105

6 months ago
Created attachment 8923452 [details] [diff] [review]
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...
Attachment #8923452 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 106

6 months ago
Created attachment 8923455 [details] [diff] [review]
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|.
Attachment #8923455 - Flags: review?(sphink)
(Assignee)

Comment 107

6 months ago
Created attachment 8923456 [details] [diff] [review]
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.
Attachment #8923456 - Flags: review?(sphink)
(Assignee)

Comment 108

6 months ago
Created attachment 8923458 [details] [diff] [review]
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. :-)
Attachment #8923458 - Flags: review?(sphink)
(Assignee)

Comment 109

6 months ago
Created attachment 8923460 [details] [diff] [review]
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.)
Attachment #8923460 - Flags: review?(jwalden+bmo)
(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.
Attachment #8923424 - Flags: review?(sphink) → review+
Attachment #8923435 - Flags: review?(sphink) → review+
Attachment #8923439 - Flags: review?(sphink) → review+
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 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+
Attachment #8923458 - Flags: review?(sphink) → review+
(Assignee)

Comment 114

6 months 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

6 months ago
Attachment #8923436 - Flags: review?(jwalden+bmo) → review+
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 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 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

6 months ago
Created attachment 8923803 [details] [diff] [review]
bug1346234-part41-move-code-into-browser-iife.patch

Update commit message from "r=Waldo" to "rs=Waldo".
Attachment #8923443 - Attachment is obsolete: true
Attachment #8923803 - Flags: review+
(Assignee)

Comment 119

6 months ago
Created attachment 8923805 [details] [diff] [review]
bug1346234-part42-browser-iife-cleanup.patch

Updated per review comments, carrying r+.
Attachment #8923452 - Attachment is obsolete: true
Attachment #8923805 - Flags: review+
(Assignee)

Comment 120

6 months ago
Created attachment 8923806 [details] [diff] [review]
bug1346234-part43-remove-optionsClear-export.patch

Needed to be rebased after changes in part 42.
Attachment #8923455 - Attachment is obsolete: true
Attachment #8923806 - Flags: review+
(Assignee)

Comment 121

6 months 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&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 123

6 months 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

6 months 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
Last Resolved: 6 months 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.