Closed
Bug 1343043
Opened 7 years ago
Closed 7 years ago
Remove global variables from shell.js which conflict with test262
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
Details
Attachments
(2 files)
6.79 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
1.81 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
We can easily remove these globals (http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/js/src/tests/shell.js#797-801) from shell.js. This will allow us to enable two additional test262 generator tests.
Assignee | ||
Comment 1•7 years ago
|
||
This removes the global variables "summary", "description", "expected", "actual", and "msg" from shell.js. Six tests used the default value (an empty string) for the "summary" variable when calling "printStatus()". I just copied the default |summary| declaration into those tests instead of removing the useless printStatus() call, because I didn't want to mingle this patch with general jstests harness clean-ups. (I have about three dozen other patches to clean-up the harness, I'll probably post them later this week.) Intl/NumberFormat/significantDigitsOfZero.js used the default values (empty strings) for |expected| and |actual| in the assertion message instead of the correct expected and actual values from test. And ecma_6/Generators/shell.js was just broken. :-/
Attachment #8841738 -
Flags: review?(shu)
Assignee | ||
Comment 2•7 years ago
|
||
Enables the previously disabled tests.
Attachment #8841739 -
Flags: review?(shu)
Comment 3•7 years ago
|
||
Comment on attachment 8841738 [details] [diff] [review] bug1343043-part1-remove-globals.patch Review of attachment 8841738 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/tests/ecma_6/Generators/shell.js @@ +3,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > function assertFalse(a) { assertEq(a, false) } > function assertTrue(a) { assertEq(a, true) } > +function assertNotEq(found, not_expected) { assertEq(Object.is(found, not_expected), false) } This is a drive-by, I suppose? Not related to the removal of the vars? ::: js/src/tests/ecma_7/TypedObject/storageopaque.js @@ +4,5 @@ > > var {StructType, uint32, Object, Any, storage, objectType} = TypedObject; > > function main() { // once a C programmer, always a C programmer. > print(BUGNUMBER + ": " + summary); I don't get this print, did it always print nothing for the summary?
Attachment #8841738 -
Flags: review?(shu) → review+
Updated•7 years ago
|
Attachment #8841739 -
Flags: review?(shu) → review+
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Shu-yu Guo [:shu] from comment #3) > Comment on attachment 8841738 [details] [diff] [review] > bug1343043-part1-remove-globals.patch > > Review of attachment 8841738 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/tests/ecma_6/Generators/shell.js > @@ +3,5 @@ > > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > > > function assertFalse(a) { assertEq(a, false) } > > function assertTrue(a) { assertEq(a, true) } > > +function assertNotEq(found, not_expected) { assertEq(Object.is(found, not_expected), false) } > > This is a drive-by, I suppose? Not related to the removal of the vars? No, it's necessary for the removal. The previous definition used |assertFalse(found === expected)|, so it was picking up the global |expected| variable from js/src/tests/shell.js instead of the parameter |not_expected|. The use of Object.is is a drive-by, though. I changed it so assertNotEq better matches the reverse of assertEq. > > ::: js/src/tests/ecma_7/TypedObject/storageopaque.js > @@ +4,5 @@ > > > > var {StructType, uint32, Object, Any, storage, objectType} = TypedObject; > > > > function main() { // once a C programmer, always a C programmer. > > print(BUGNUMBER + ": " + summary); > > I don't get this print, did it always print nothing for the summary? Yes, and I've seen worse while doing the other clean-ups for the jstests tests. :-)
Assignee | ||
Comment 5•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51963a080500b0636bf3a5d4ab350c481a311098
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/36040e953137 Part 1: Remove some global variables from shell.js and instead declare them locally in tests. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/83854fc82f3d Part 2: Enable previously disabled test262 tests. r=shu
Keywords: checkin-needed
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/36040e953137 https://hg.mozilla.org/mozilla-central/rev/83854fc82f3d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•