-
Notifications
You must be signed in to change notification settings - Fork 474
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
Replace checks in old tests with assert.sameValue #3868
base: main
Are you sure you want to change the base?
Conversation
c507ff1
to
c6d2d40
Compare
@CanadaHonk, this is amazing! Thanks. I think it'll be quite difficult to review effectively by hand, so my preference would be that we just rely on the ESMeta checks, and possibly ask an implementation to run the test suite from your branch. I'm curious what the other maintainers think though. It looks like there are a few failures in the ESMeta job in the last run, so would you be able to take a look at those before proceeding? |
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 ran test262 with LibJS against this branch and got the following results:
Diff Tests:
test/built-ins/Array/prototype/sort/bug_596_2.js ✅ -> ❌
test/language/expressions/unary-minus/S11.4.7_A3_T1.js ✅ -> ❌
test/language/expressions/unary-minus/S11.4.7_A3_T4.js ✅ -> ❌
test/language/line-terminators/between-tokens-cr.js ✅ -> ❌
test/language/line-terminators/comment-multi-cr.js ✅ -> ❌
test/built-ins/Array/prototype/sort/bug_596_2.js
has a syntax error (assert.sameValue(array.hasOwnProperty('0'), true, '#2: array.hasOwnProperty('0')');
)test/language/expressions/unary-minus/S11.4.7_A3_T1.js
andtest/language/expressions/unary-minus/S11.4.7_A3_T4.js
compare-0
with0
which worked before with===
but no longer does withassert.sameValue()
test/language/line-terminators/between-tokens-cr.js
andtest/language/line-terminators/comment-multi-cr.js
got their line terminators messed up
Yeah, this still has some issues with a few tests hence draft ;) Will fix today. Thanks both for feedback :) |
dc5be8d
to
98924be
Compare
Fixed comments from Linus (thanks for details), will check results of engines with new changes. |
FYI, the results of the CI jobs don't say whether any tests failed (it's often expected that tests will fail in engines because we merge tests for spec changes in advance of implementations catching up) but only that the engine was able to execute them. That's why Linus' comment was so helpful 😄 |
Many old tests use custom checks with if and throwing a custom error instead of using `assert.sameValue`, this replaces most cases.
98924be
to
aed876c
Compare
@CanadaHonk It's been a while, so sorry for the delay, but could you check the ESMeta results again? Since this is a long and repetitive PR we should make sure that we're using all of the possibilities for automation that are at our disposal! Here are the failures reported by the job:
|
Many old tests use custom checks with if and throwing a custom error instead of using
assert.sameValue
, this replaces most cases.