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

Initial snapshot testing #317

Merged
merged 3 commits into from
Dec 10, 2020
Merged

Initial snapshot testing #317

merged 3 commits into from
Dec 10, 2020

Conversation

grovesNL
Copy link
Collaborator

@grovesNL grovesNL commented Dec 8, 2020

Fixes #41

Snapshots are committed to the repository so they can be compared as we update readers, writers, and examples. The idea is that we can keep testing against known working states to tell if we improved or regressed some functionality as we make changes to the codebase.

Currently the snapshots in this PR cover WGSL being read and outputting MSL and SPIR-V, but we can use them for snapshotting basically anything (though it's most useful on text). In the test in this PR, we read writer options for MSL and SPIR-V from params.ron, like the convert example does currently. We also disassemble SPIR-V output so it's easier to compare changes.

Here are the steps to using snapshot tests:

  1. First we can create a new test if there is no test covering the functionality we're interested in snapshotting
    #[cfg(feature = "wgsl-in")]
    #[test]
    fn converts_quad_wgsl() {
        convert_wgsl("quad")
    }
    
  2. In the test, somewhere we'll use insta::assert_snapshot! which compares the output vs. the existing snapshot. If there is no existing snapshot, one will be created.
  3. Then we can run cargo test as usual and follow the directions for insta if we want to update snapshots. We can use insta's review feature (e.g. cargo insta test --review --all-features --all) to diff any snapshots that have changed, and choose whether to accept or reject each change. If we accept the change, the new snapshot should be committed to the repository for future testing.
  4. Iterate using the snapshot tests – make changes to the writers or examples, keep rerunning the snapshot tests and review.

Some open questions:

  • Is this useful? We can optionally drop insta later if turns out that we'd prefer to do these ad-hoc or some other way.
  • How should we structure test-data vs. snapshots?
  • Which parts should we snapshot initially (e.g. MSL output, SPV output, IR)?
  • Should we use the existing params.ron deserialization from the convert example, or write it inline in tests? If we want to share it, where should we do that?

Copy link
Contributor

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

This is an autogenerated code review.New suggestions: 3

tests/snapshots.rs Outdated Show resolved Hide resolved
tests/snapshots.rs Outdated Show resolved Hide resolved
tests/snapshots.rs Outdated Show resolved Hide resolved
@grovesNL grovesNL marked this pull request as draft December 8, 2020 19:47
@grovesNL grovesNL marked this pull request as ready for review December 9, 2020 04:48
Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

That sounds like fun, let's try it out!
Just one concern about the leftover tests/convert.rs, please feel free to address and/or land.

tests/snapshots.rs Show resolved Hide resolved
}

#[derive(Hash, PartialEq, Eq, Deserialize)]
struct BindSource {
Copy link
Member

Choose a reason for hiding this comment

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

We need to find a way to share these structures, at least with convert.rs but ideally with Naga backends.
I explored the latter and so far failed, basically got blocked on rust-lang/cargo#4663
Anyway, we can follow-up with that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, I was also wondering if we should have an Options struct per backend (msl-out already has msl::Options) and serialize/deserialize those options directly

Copy link
Member

Choose a reason for hiding this comment

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

Yes, so the problem here is - "when would Serialize/Deserialize be implemented on these Option structs?". Obviously, from Naga point of view, these implementations would be behind some feature, be it just serde, or the more specific "serialize"/"deserialize". However, from the convert example point of view, forcing the user to always enable this feature is an unwanted burden, i.e. we want the users to write:

cargo run --example convert --features msl-in,spv-out

and not

cargo run --example convert --features serde-options,msl-in,spv-out

This problem would be easily solved by rust-lang/cargo#4663, but it appears to still be a long way to go...

Copy link
Collaborator Author

@grovesNL grovesNL Dec 9, 2020

Choose a reason for hiding this comment

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

Ah ok, got it. If we remove convert.rs and only use it from these snapshot tests, could we use something like #[cfg_attr(test, derive(Serialize, Deserialize))] on Option?

Copy link
Member

Choose a reason for hiding this comment

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

That's a good idea, we should do this for tests regardless of how "examples/convert.rs" does it, but we'll need to preserve RON format compatibility between the "examples/convert.rs" expectations and the tests.

@kvark kvark merged commit d71ebe2 into gfx-rs:master Dec 10, 2020
@grovesNL grovesNL deleted the snapshots branch December 10, 2020 05:27
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.

Integrate with snapshot testing
2 participants