Update iteration tests with respect to spec changes #1248
Conversation
|
picked out tests which need changing from https://chromium-review.googlesource.com/c/v8/v8/+/687997/4/test/test262/test262.status#544 I'll also add a PR for the async iteration tests which were affected seperately. |
| @@ -3,39 +3,33 @@ | |||
| /*--- | |||
| es6id: 13.6.4.13 S5.c | |||
There was a problem hiding this comment.
can you replace this with the current esid? This test doesn't reflect the ES6 specs anymore.
| }; | ||
| Object.defineProperty(iterator, 'next', { | ||
| get: function() { | ||
| $ERROR('Should not access the `next` method after the iteration ' + |
There was a problem hiding this comment.
while you're here, would you mind to replace this $ERROR call to a throw new Test262Error?
This should not be a blocker.
| iterator.next = function() { return { value: 45, done: false }; }; | ||
| iterationCount = 0; | ||
| invocationCount = 0; | ||
| iterator.next = function() { |
There was a problem hiding this comment.
maybe in a follow up PR - which I can do - or here if you're up to: defining this over a Object.defineProperty(iterator, 'next', get accessor should be better for catching it get's the method only once.
Again, this should not be a blocker.
There was a problem hiding this comment.
Is this in addition to the defineProperty below, or on its own?
There was a problem hiding this comment.
the changes you did sounds great already.
tc39/ecma262#988 changes the iteration protocol such that the "next" method is only loaded from the iterator object once during the prologue of iteration, rather than during each step.
|
Is this the only test which changes due to the new iteration protocol? It might be a good follow-on to add some more tests, e.g., for spread, Array.from, Promise.all, and anyplace else that the iteration protocol is used. The parts of the spec updated in the patch could be a good guide. |
If the tests are correct there should be no blocker to merge them.
We should probably find a list of all places this can also be observed. Are you interested to assemble this in a new issue? Also, we have this extra coverage: #1250
yes, fetch by GetIterator and IteratorNext calls are a good start |
I'm not trying to block anything, just giving an area for future tests to be written.
I'd recommend that whoever writes this patch goes through tc39/ecma262#988 and writes a test for each bunch of code affected. It's great that there are async iteration tests, but that's not the only place. I don't think I'll be able to do this anytime soon, unfortunately. |
Since ES2018 iterator's next method is called once during the prologue of iteration, rather than during each step. The test is incorrect and stuck in an infinite loop. tc39/test262#1248 fixed the test and it passes on test262-esnext. Removing test/language/statements/for-of/iterator-next-reference.js from test262-es2015 is necessary, because it won't pass ever and only slow down testing. JerryScript-DCO-1.0-Signed-off-by: Csaba Osztrogonác [email protected]
Since ES2018 iterator's next method is called once during the prologue of iteration, rather than during each step. The test is incorrect and stuck in an infinite loop. tc39/test262#1248 fixed the test and it passes on test262-esnext. Removing test/language/statements/for-of/iterator-next-reference.js from test262-es2015 is necessary, because it won't pass ever and only slow down testing. JerryScript-DCO-1.0-Signed-off-by: Csaba Osztrogonác [email protected]
tc39/ecma262#988 changes the iteration protocol
such that the "next" method is only loaded from the iterator object once
during the prologue of iteration, rather than during each step.