Skip to content

Tests are broken #3204

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.

Sign up for GitHub

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

Jump to bottom
Closed
uberFoo opened this issue Dec 18, 2022 · 2 comments
Closed

Tests are broken #3204

uberFoo opened this issue Dec 18, 2022 · 2 comments
Labels
bug

Comments

@uberFoo
Copy link

uberFoo commented Dec 18, 2022 •
edited
Loading

I was going to start hacking on enum support this morning. Before I broke stuff I figured I should run the tests: warm-pack test --node, and they failed. I bisected the repo and found:

╭─[email protected] ~/projects/github/wasm-bindgen ‹3.1.3› ‹ebe65873› 
╰─➤  git bisect good      
df1b591a1cb88f0255844d26b13f5364fdb0950e is the first bad commit
commit df1b591a1cb88f0255844d26b13f5364fdb0950e
Author: João Freires <[email protected]>
Date:   Wed Jun 22 17:44:52 2022 -0300

It was introduced in #2954. I haven't really looked into it much, but I have a bad feeling that fixing this is going to involve a lot of changes to the tests themselves. I'm going to look into it more.

TBH, I'm a bit surprised that nobody has even noticed.
@uberFoo uberFoo added the bug label Dec 18, 2022
@Liamolucko
Copy link
Collaborator

That's not how the tests are meant to be run. See https://rustwasm.github.io/wasm-bindgen/contributing/testing.html; the main set of tests are run with cargo test --target wasm32-unknown-unknown.

As for why they're failing like that, it's because wasm-pack is attempting to use the latest published version of the CLI with the current git version of the macro, which is incompatible. Using cargo test builds the current git version of the CLI from source instead.

@uberFoo
Copy link
Author

uberFoo commented Dec 19, 2022

It seemed likely I was doing something wrong. I found an old README in the repo that led me to believe that the two ways of running tests were similar. I went the wrong way because of some other problem running the tests with cargo. I'll have to go back and look into that.

Thanks!

Liamolucko added a commit to Liamolucko/wasm-bindgen that referenced this issue Dec 19, 2022
@Liamolucko
Validate the schema version before processing descriptors
9522dec 
While looking into rustwasm#3204 I noticed that the error given by `wasm-pack test` was a panic while decoding descriptors, rather than the 'version mismatch' error that using an incompatible CLI version should give. It turns out that that was happening because descriptors were getting parsed before the custom section, and hence before the version check.

So, I reordered things so that the custom section is parsed (but not processed) before the descriptors, which includes checking the schema version. The custom section still gets processed afterwards, though, since that seems to depend on the results of processing descriptors.
@Liamolucko Liamolucko mentioned this issue Dec 19, 2022
Merged
alexcrichton pushed a commit that referenced this issue Jan 3, 2023
@Liamolucko
Validate the schema version before processing descriptors (#3205)
ee89f90 
While looking into #3204 I noticed that the error given by `wasm-pack test` was a panic while decoding descriptors, rather than the 'version mismatch' error that using an incompatible CLI version should give. It turns out that that was happening because descriptors were getting parsed before the custom section, and hence before the version check.

So, I reordered things so that the custom section is parsed (but not processed) before the descriptors, which includes checking the schema version. The custom section still gets processed afterwards, though, since that seems to depend on the results of processing descriptors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@uberFoo @Liamolucko