-
Notifications
You must be signed in to change notification settings - Fork 451
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
Use node-canary to run interpreter-generated JS tests #1833
base: wasm-3.0
Are you sure you want to change the base?
Conversation
f607d1f
to
972e11f
Compare
Don't we need an opt-out mechanism to be able to land tests that node/V8 doesn't currently pass? |
Yes, good point. The comment I'm replacing here seems to suggest that just commenting this line out with a TODO is one way to skip these days when working on such a proposal. Perhaps we should separate the "run tests in interp" and "run tests under node/v8" so that its easy to comment out just the later? |
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.
A proper opt-out mechanism for in the test runner would be useful. We also talked about this in the context of individual (negative) tests that conflict with a yet unfinished proposal. But designing and implementing this properly seems like a lot of work.
As for separate targets, wouldn't that still involve commenting out a line?
.github/workflows/ci-interpreter.yml
Outdated
NODE=$HOME/node-v${NODE_VERSION}-linux-x64/bin/node | ||
cd interpreter && opam exec make "JS=$NODE --wasm-staging" ci |
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.
NODE=$HOME/node-v${NODE_VERSION}-linux-x64/bin/node | |
cd interpreter && opam exec make "JS=$NODE --wasm-staging" ci | |
NODE="$HOME/node-v${NODE_VERSION}-linux-x64/bin/node --wasm-staging" | |
cd interpreter && opam exec make JS=$NODE ci |
Not sure I see how. AFAICS, it does not bring us any closer. I mean, I'm not opposed, but I don't think it makes a difference one way or the other, other than complicating the test runner in different ways. In any case, I think neither should block the current PR, which still is a net improvement. |
It seems like |
Apparently there is a fix in v8 that landed just this morning that might be fix this, so I guess we are already running into the issue of needing to selectively disable tests. I'll give this PR another try once a new node-canary is published. |
No description provided.