-
Notifications
You must be signed in to change notification settings - Fork 194
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
Conversation
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.
This is an autogenerated code review.New suggestions: 3
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.
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.
} | ||
|
||
#[derive(Hash, PartialEq, Eq, Deserialize)] | ||
struct BindSource { |
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.
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.
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.
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
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.
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...
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.
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
?
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.
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.
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:
insta::assert_snapshot!
which compares the output vs. the existing snapshot. If there is no existing snapshot, one will be created.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.Some open questions:
insta
later if turns out that we'd prefer to do these ad-hoc or some other way.convert
example, or write it inline in tests? If we want to share it, where should we do that?