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

Properly test wasm32-unknown-unknown on CI #206

Closed
wants to merge 10 commits into from

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Jul 9, 2019

Currently not all configurations are build for wasm32, and the llvm intrinsics are incorrectly feature gated.

@alexcrichton
Copy link
Member

I don't think this is something we want to do on CI? Wasm is different enough that we don't really want to test all configurations exhaustively, we just want to make sure that it builds at all and doesn't hit codegen errors. The CI errors here for wasm are sort of just the start I'd imagine for getting the full suite running.

@gnzlbg gnzlbg force-pushed the wasm32 branch 4 times, most recently from 4e01576 to a9a76f3 Compare July 9, 2019 16:40
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 10, 2019

So this is ready for now. Depending on whether #204 is merged or not, what would remain doing is using the wasm_bindgen_test macro. I find it would be easier to do when all tests are in the same file.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 11, 2019

So I've added use of wasm_bindgen_test here. As you can see, I had to add a #![cfg(not(test))] to the compiler_builtin_smoke test to avoid including the tests.

I can workaround that here if you want, but the more tests we add directly into the libm crate, the more workarounds are going to be required in the compiler_builtin_smoke crate, and in the compiler-builtins crate, to either duplicate the testing framework of libm so that compiler_builtins can be tested, or to silence including the tests there.

I still think that, since this crate is only used by including it via #[path], the cleanest solution is to have libm be just the code that gets included, and to move all the tests to a different crate, to avoid having to add workarounds for this all over the place. In the mean time, #![cfg(not(test))] works for the compiler builtins smoke test, and would also be a one line change for the compiler-builtin test, although disabling all tests there might not be desired.

I suppose we could add --cfgs here that only enable stuff when testing, but from the experience with #198 , we are going to need quiet a few of them (for exhaustiveness tests, mpfr tests, etc.).

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 11, 2019

Note: the failures here are unrelated and due to rust-lang/rust#62574 , should be fixed in nightly soon.

@alexcrichton
Copy link
Member

I'm personally not really super keen on merging this unfortunately. This seems to add a good deal of implementation complexity and infrastructure, which is just yet-again-more to keep working on CI. I don't think we really get any benefit from running these tests on wasm either, so I'm not really sure it's worth the weight of this PR to merge it?

@gnzlbg gnzlbg closed this Jul 12, 2019
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