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

Disable test_autodebug for V8 #4633

Merged

Conversation

jgravelle-google
Copy link
Contributor

test_autodebug is inaccurately marked no_wasm_backend(), when really it
doesn't work in v8, with either backend or asm2wasm.
v8 does work with asmjs, but just banning v8 is more accurate than not
disabling this test for asm2wasm.

We'd like to test asm2wasm against the emscripten testsuite on the wasm waterfall using v8, and this is the only failing test in that case.

test_autodebug is inaccurately marked no_wasm_backend(), when really it
doesn't work in v8, with either backend or asm2wasm.
v8 does work with asmjs, but just banning v8 is more accurate than not
disabling this test for asm2wasm.
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with that change

def test_autodebug(self):
self.banned_js_engines = [V8_ENGINE] # Broken on V8 but not node; wasm-only
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is wasm-only, then this could be if self.is_wasm(): and then your new line. then we'd still test v8 on the asm.js version.

@jgravelle-google
Copy link
Contributor Author

Best of both worlds
Done

@kripken
Copy link
Member

kripken commented Oct 18, 2016

Thanks. One last thing - is there a v8 bug open for this? If so would be nice to add the URL in the comment.

@jgravelle-google
Copy link
Contributor Author

Realized I forgot to follow up on this: I don't think there's a V8 bug open, though to be honest I'm not entirely sure what's causing the breakage.

Meanwhile the change as it was didn't actually cause the test to not-fail, because it would trip on the asserts immediately afterward. 96fc2c4 should address that.

@kripken kripken merged commit 931ffe3 into emscripten-core:incoming Oct 20, 2016
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

Successfully merging this pull request may close these issues.

2 participants