-
Notifications
You must be signed in to change notification settings - Fork 494
Start cleaning up staging/sm tests #4463
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed up to "Remove non262-extensions-shell.js"
@@ -10,7 +10,7 @@ function assertFalse(a) { assertEq(a, false) } | |||
function assertTrue(a) { assertEq(a, true) } | |||
function assertNotEq(found, not_expected) { assertEq(Object.is(found, not_expected), false) } | |||
function assertIteratorResult(result, value, done) { | |||
assertDeepEq(result.value, value); | |||
assert.deepEqual(result.value, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ms2ger Does your importation script need to be updated to make this replacement as well?
test/staging/sm/strict/15.4.5.1.js
Outdated
try | ||
{ | ||
strict1(out); | ||
throw "no error"; | ||
} | ||
catch (e) | ||
{ | ||
assert.sameValue(e instanceof TypeError, true, "expected TypeError, got " + e); | ||
} | ||
assert.sameValue(deepEqual(out.array, [1, 2, 3]), true); | ||
|
||
// Internally, SpiderMonkey has two representations for arrays: | ||
// fast-but-inflexible, and slow-but-flexible. Adding a non-index property | ||
// to an array turns it into the latter. We should test on both kinds. | ||
function addx(obj) { | ||
obj.x = 5; | ||
return obj; | ||
} | ||
|
||
function nonStrict2(out) | ||
{ | ||
var a = out.array = addx(arr()); | ||
a.length = 2; | ||
} | ||
|
||
function strict2(out) | ||
{ | ||
"use strict"; | ||
var a = out.array = addx(arr()); | ||
a.length = 2; | ||
} | ||
|
||
out.array = null; | ||
nonStrict2(out); | ||
assert.sameValue(deepEqual(out.array, addx([1, 2, 3])), true); | ||
|
||
out.array = null; | ||
try | ||
{ | ||
strict2(out); | ||
throw "no error"; | ||
} | ||
catch (e) | ||
{ | ||
assert.sameValue(e instanceof TypeError, true, "expected TypeError, got " + e); | ||
} | ||
assert.sameValue(deepEqual(out.array, addx([1, 2, 3])), true); | ||
assert.throws(TypeError, function() { | ||
strict1(out); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess maybe in the interest of max coverage of implementation strategies, we shouldn't necessarily remove this just because SpiderMonkey updated their implementation details? Making an array slow when adding a non-index property seems like a choice that other implementations might make.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file tests that modifying length
fails for an array with a non-configurable array element. If we want to have a test to cover that arrays support non-index properties, we shouldn't use this file for that. (And we probably already have coverage that arrays support non-index properties.)
The test itself was added in https://bugzilla.mozilla.org/show_bug.cgi?id=537873. And the "slow array" part became obsolete twelve years ago in https://bugzilla.mozilla.org/show_bug.cgi?id=827490.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed up to "Replace makeIterator with inline definitions"
…in-first-expression.js
…ntains-unicode-escape.js and fix a test bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed up to "Remove assertThrownErrorContains"
|
||
// Yield every permutation of the elements in some array. | ||
function* Permutations(items) { | ||
if (items.length == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good practice to replace this with ===
while we're here
let sorted = permutation.sort(comparefn); | ||
assert.compareArray(sorted, data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorted
is in fact now the same array as permutation
, better to just use permutation
to avoid confusion
|
||
// Yield every permutation of the elements in some array. | ||
function* Permutations(items) { | ||
if (items.length == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, replace with ===
@@ -30,7 +29,7 @@ function testName(thisv) { | |||
assertThrowsInstanceOf(() => String.prototype[key].call(thisv), TypeError, key); | |||
} else { | |||
var expected = `String.prototype.${key} called on incompatible ${thisv}`; | |||
assertThrowsInstanceOfWithMessage(() => String.prototype[key].call(thisv), TypeError, expected, key) | |||
assert.throws(TypeError, () => String.prototype[key].call(thisv), TypeError, expected) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra TypeError
in argument position 3
Changes:
noStrict
flag where possible.I've also identified a couple of tests which should be removed from test262, because they test non-standard behaviour:
Function.caller
.Date.parse
with non-standard inputs.Math
functions are.disassemble
ortimeout
.In a follow-up PR I can remove these tests, too.