Skip to content
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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

CanadaHonk
Copy link

Many old tests use custom checks with if and throwing a custom error instead of using assert.sameValue, this replaces most cases.

@CanadaHonk CanadaHonk force-pushed the replace-old-assert-samevalue branch 2 times, most recently from c507ff1 to c6d2d40 Compare July 8, 2023 12:39
@ptomato
Copy link
Contributor

ptomato commented Jul 11, 2023

@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?

Copy link
Member

@linusg linusg left a 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 and test/language/expressions/unary-minus/S11.4.7_A3_T4.js compare -0 with 0 which worked before with === but no longer does with assert.sameValue()
  • test/language/line-terminators/between-tokens-cr.jsand test/language/line-terminators/comment-multi-cr.js got their line terminators messed up

@CanadaHonk
Copy link
Author

Yeah, this still has some issues with a few tests hence draft ;) Will fix today. Thanks both for feedback :)

@CanadaHonk CanadaHonk force-pushed the replace-old-assert-samevalue branch 3 times, most recently from dc5be8d to 98924be Compare July 11, 2023 13:50
@CanadaHonk
Copy link
Author

Fixed comments from Linus (thanks for details), will check results of engines with new changes.

@ptomato
Copy link
Contributor

ptomato commented Jul 11, 2023

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.
@ptomato ptomato force-pushed the replace-old-assert-samevalue branch from 98924be to aed876c Compare October 25, 2023 17:22
@ptomato
Copy link
Contributor

ptomato commented Oct 25, 2023

@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:

[Test262Error] 9 tests are failed:
built-ins/String/S15.5.1.1_A1_T16.js - Invalid operation
built-ins/String/S15.5.1.1_A1_T17.js - Invalid operation
built-ins/String/S9.8.1_A10.js - Invalid operation
built-ins/String/S9.8.1_A7.js - Invalid operation
built-ins/String/S9.8.1_A8.js - Invalid operation
built-ins/String/S9.8.1_A9_T2.js - Invalid operation
built-ins/String/prototype/lastIndexOf/S15.5.4.8_A1_T7.js - [InterpreterError] return not undefined: comp[~throw~](#2203)
built-ins/String/prototype/slice/S15.5.4.13_A1_T15.js - Invalid operation
built-ins/String/prototype/substring/S15.5.4.15_A1_T15.js - Invalid operation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants