-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 snapshot tests #4047
base: main
Are you sure you want to change the base?
Add snapshot tests #4047
Conversation
c68cb30
to
9473739
Compare
79587ce
to
ddf62b7
Compare
47b7394
to
90dfbfe
Compare
This comment was marked as outdated.
This comment was marked as outdated.
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 big issue with the current testing infrastructure in wasm-bindgen
is that is terribly fragmented.
While I believe this PR is a great change, it further increases that fragmentation.
Currently similar tests are done in /crates/cli/tests/reference
.
Would it be possible to migrate the current similar tests to use insta
as well?
I didn't look to closely into insta
yet, so let me know if its just not possible or why you believe it shouldn't be done.
Again, thank you!
I believe this is a great improvement for wasm-bindgen
.
cf818b7
to
3795b6b
Compare
There are other similar ones in the |
5a6497b
to
879d25c
Compare
6ff7a67
to
d6e0833
Compare
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.
Currently similar tests are done in /crates/cli/tests/reference.
Would it be possible to migrate the current similar tests to use insta as well?There are other similar ones in the
xform
tests, but I'm not sure. I don't know what theBLESS
environment variable is supposed to do exactly. From a glance I thinkinsta
can be used for the tests that compare against a reference; I'd prefer not to get into it though.
BLESS
fixes the references to adhere to the output instead of checking against it. This is used when the output is actually modified and you want to update all the references. Its like INSTA_UPDATE="always"
I assume.
Unless there is a good reason to introduce a second testing system instead of using the one in place, I don't want to further fragment our testing setup.
I am happy too replace the system we currently have in place, but having multiple once in place at the same time introduces friction into the contributor experience.
CI improvements should be done in separate PR.
I pulled out the other CI changes, will send a PR with less duplication.
I'll leave that to you or anyone else that wants to pick it up from here in that case, this would just be a small addition to make reviewing refactoring and target addition PRs easier anyway. |
I think snapshot tests are a great choice here since they make both the current state as well as future changes easily observable.
insta
errors with a descriptive diff and the snapshots are easy to generate during development. For updating snapshots, you can use the recommended workflow or simply setINSTA_UPDATE="always"
to overwrite the existing ones when running the tests.The snapshots themselves are currently not ideal due to the way the output is assembled, but I want to keep refactoring for another PR.