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

Add a typescript test (not just a compile test) for enums #3740

Merged
merged 3 commits into from
Feb 8, 2024

Conversation

thomasetter
Copy link
Contributor

Also changed from opt-in to opt-out for running TypeScript tests.

This will make sure that new tests are run by default.

@thomasetter thomasetter force-pushed the more-ts-tests branch 2 times, most recently from 9c6f801 to 440191f Compare January 1, 2024 12:25
@thomasetter
Copy link
Contributor Author

Friendly ping :)

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

LGTM!

@Liamolucko would you mind taking a look at this? Unfortunately I'm completely unfamiliar with TS, so I don't feel comfortable approving something like this.

Copy link
Collaborator

@Liamolucko Liamolucko left a comment

Choose a reason for hiding this comment

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

Personally I don't think this is super necessary, I think a compile test is enough: making sure that wasm-bindgen works properly at runtime is already covered by the regular tests in tests/wasm.

Having extra tests doesn't hurt though, so I'm fine to merge this regardless.

crates/typescript-tests/src/enums.ts Outdated Show resolved Hide resolved
@thomasetter
Copy link
Contributor Author

Thanks, just for the motivation why I am doing this:
The problem with compile-only tests is that they just assert properties of the binding (.d.ts), however the binding and the underlying JS can (and I believe in the past have, but do not remember the location/commits) drift apart, which will then only be discovered by users after a release.

Copy link
Collaborator

@Liamolucko Liamolucko left a comment

Choose a reason for hiding this comment

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

Thanks!

@Liamolucko Liamolucko merged commit 114a4a1 into rustwasm:main Feb 8, 2024
25 checks passed
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.

3 participants