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 snapshot tests #4047

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

Systemcluster
Copy link
Contributor

@Systemcluster Systemcluster commented Aug 4, 2024

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 set INSTA_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.

@Systemcluster Systemcluster force-pushed the insta branch 2 times, most recently from c68cb30 to 9473739 Compare August 4, 2024 04:28
@daxpedda daxpedda marked this pull request as draft August 4, 2024 06:51
@Systemcluster Systemcluster force-pushed the insta branch 2 times, most recently from 79587ce to ddf62b7 Compare August 4, 2024 09:36
@Systemcluster Systemcluster marked this pull request as ready for review August 4, 2024 10:00
@Systemcluster Systemcluster force-pushed the insta branch 3 times, most recently from 47b7394 to 90dfbfe Compare August 4, 2024 21:16
@Systemcluster

This comment was marked as outdated.

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.

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.

crates/cli-support/tests/test_add.rs Outdated Show resolved Hide resolved
crates/cli-support/Cargo.toml Outdated Show resolved Hide resolved
@daxpedda daxpedda added the waiting for author Waiting for author to respond label Aug 5, 2024
@Systemcluster Systemcluster force-pushed the insta branch 7 times, most recently from cf818b7 to 3795b6b Compare August 8, 2024 03:36
@Systemcluster
Copy link
Contributor Author

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 the BLESS environment variable is supposed to do exactly. From a glance I think insta can be used for the tests that compare against a reference; I'd prefer not to get into it though.

@Systemcluster Systemcluster changed the title Add snapshot tests Add snapshot tests & other CI improvements Aug 8, 2024
.npmrc Outdated Show resolved Hide resolved
@Systemcluster Systemcluster force-pushed the insta branch 3 times, most recently from 5a6497b to 879d25c Compare August 8, 2024 06:49
@Systemcluster Systemcluster force-pushed the insta branch 6 times, most recently from 6ff7a67 to d6e0833 Compare August 8, 2024 07:37
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.

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 the BLESS environment variable is supposed to do exactly. From a glance I think insta 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.

.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
@Systemcluster
Copy link
Contributor Author

I pulled out the other CI changes, will send a PR with less duplication.

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'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.

@Systemcluster Systemcluster changed the title Add snapshot tests & other CI improvements Add snapshot tests Aug 8, 2024
@Systemcluster Systemcluster marked this pull request as draft August 8, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for author Waiting for author to respond
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants