Change tests for extends null and Intl legacy constructor semantics#855
Change tests for extends null and Intl legacy constructor semantics#855leobalter merged 7 commits intotc39:masterfrom
extends null and Intl legacy constructor semantics#855Conversation
littledan
left a comment
littledan
left a comment
There was a problem hiding this comment.
All of these changes look great. Thanks for your attention to detail in updating the tests for extending null, as well as fixing that SAB test bug (I ran into this when trying to do apply the latest test262 tests but didn't follow up nicely as you did).
I'm working on tests for the constructor path (I guess for complete tests we'll have to figure out what the [[Description]] should be), by the way.
| /*--- | ||
| es5id: 12.1.1_1 | ||
| description: Tests that the this-value is ignored in DateTimeFormat. | ||
| description: Tests that the this-value is ignored in DateTimeFormat, if the this-value isn't a DateTimeFormat instance. |
There was a problem hiding this comment.
Optional nit: this could be word-wrapped.
anba
force-pushed
the
fix-async-sab-class-tests
branch
from
1b6b382 to
878d382
Compare
|
Rebased and merged the fixup commit. Also added a new commit to fix typos in the new built-ins/TypedArray/prototype/copyWithin tests. |
| assert(calledExecutor); | ||
| assert.sameValue(executorArguments.length, 2); | ||
| assert.sameValue(typeof executorArguments[0], "function"); | ||
| assert.sameValue(typeof executorArguments[1], "function"); |
There was a problem hiding this comment.
so much better testing this way!
| var f = new Foo(); | ||
|
|
||
| assert.sameValue(f, obj); | ||
| assert.sameValue(Object.getPrototypeOf(f), Object.prototype); |
There was a problem hiding this comment.
this second assertion became unnecessary, but it does not hurt keeping it.
| constructor() { | ||
| reachable += 1; | ||
| super(); | ||
| super(unreachable += 1); |
| class Foo extends null { | ||
| constructor() {} | ||
| constructor() { | ||
| } |
There was a problem hiding this comment.
in a follow up PR, we could verify this is called. the reachable += 1 approach would be good enough
| assert.sameValue(instance, thisVal); | ||
| assert.throws(ReferenceError, function() { | ||
| new C(); | ||
| }); |
There was a problem hiding this comment.
again a suggestion for a follow up PR: this is not what we are really asserting in this test, so a try catch block would avoid ambiguity.
| // Use an arrow function to access the `this` binding of the class constructor. | ||
| assert.throws(ReferenceError, () => { | ||
| this; | ||
| }); |
|
Everything is looking great here, I found a few things that could be changed/improved but it's not necessary to block this PR. |
3 participants
The main changes are for tc39/ecma262#781 and tc39/ecma402#84, plus some other small bug fixes.
tc39/ecma402#84 also needs new tests, but I'd like to fix the failing tests first.