Closed Bug 1343043 Opened 3 years ago Closed 3 years ago

Remove global variables from shell.js which conflict with test262

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: anba, Assigned: anba)

Details

Attachments

(2 files)

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.
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)
Enables the previously disabled tests.
Attachment #8841739 - Flags: review?(shu)
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+
Attachment #8841739 - Flags: review?(shu) → review+
(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. :-)
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
You need to log in before you can comment on or make changes to this bug.