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

staging/sm has a lot of tests that don't use the test262 harness #4376

Open
syg opened this issue Jan 10, 2025 · 4 comments
Open

staging/sm has a lot of tests that don't use the test262 harness #4376

syg opened this issue Jan 10, 2025 · 4 comments
Assignees

Comments

@syg
Copy link
Contributor

syg commented Jan 10, 2025

There are calls to gc, assertEq, etc. One of the requirements to landing to staging/ is that the tests are runnable with the usual test262 harness.

@Ms2ger Can SM's harness-isms be linted against in the future to avoid landing tests that don't run?

@Ms2ger Ms2ger self-assigned this Jan 13, 2025
@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 13, 2025

Looks like gc() slipped through the cracks indeed. assertEq was caught in the tests, but annoyingly not in the harness files. I've compared results with V8, so I'm confused why I didn't catch those. In any case I'll get them fixed.

@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 13, 2025

Actually, assertEq should be covered by harness/sm/non262.js. Can you give more details on what goes wrong?

@syg
Copy link
Contributor Author

syg commented Jan 14, 2025

Actually, assertEq should be covered by harness/sm/non262.js. Can you give more details on what goes wrong?

Did some digging. The issue is that the order of files in the includes: array matters (at least for V8's test runner, but I imagine it does for everyone?), and sm/non262.js is being included too late in many (all?) of the tests.

To take a concrete example, sign.js#L5 has includes: [sm/non262-Math-shell.js, sm/non262-shell.js, sm/non262.js]. sm/non262-Math-shell.js#L54 calls assertEq on the toplevel, but at that point non262.js hasn't been run yet, so it fails.

If the includes: lines put sm/non262.js first, then it works.

Ms2ger added a commit to Ms2ger/test262 that referenced this issue Jan 14, 2025
@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 15, 2025

Oh, I finally figured out why I didn't catch it. test262-harness just concatenates the included files, and the function declaration is hoisted, so assertEq was available even though the includes were in the wrong order. I'll try to get a PR to reorder the includes up asap, and add documentation that they should be evaluated in order.

Ms2ger added a commit that referenced this issue Jan 15, 2025
ptomato pushed a commit that referenced this issue Jan 15, 2025
Ms2ger added a commit that referenced this issue Jan 16, 2025
Ms2ger added a commit that referenced this issue Jan 17, 2025
Ms2ger added a commit that referenced this issue Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants