-
Notifications
You must be signed in to change notification settings - Fork 667
Add snapshot tests for normalized ModuleDef #1619
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
Conversation
|
That's fine by me; I don't think it will be hard to adapt tests either way. |
| } | ||
|
|
||
| for table in &mut tables { | ||
| // Rust generates same number of constraints as number of columns, |
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.
@kazimuth Is this something that will be gone in the new schema by any chance?
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.
It is indeed! We still generate indexes for unique constraints, but all the other generated stuff is gone.
e7bcc4d to
cac47b0
Compare
|
@kazimuth I rebased this, but on the 2nd thought we shouldn't compare V9 moduledef just yet. The purpose of the original issue / of adding those tests was to verify that the module structure used for generation is the same across different implementations (Rust and C#) of the same module. Since Meanwhile, we'll at least already have some testing in place. |
kazimuth
left a comment
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.
I like the new debug formatting for types, that's very convenient.
I'm happy merging this for now and updating it once the stuff it depends on has been rewritten. I agree that it kind of doesn't make sense to test all the new stuff while it hasn't been fully plumbed through everything.
fa297de to
6a4c8a7
Compare
6a4c8a7 to
19de8cb
Compare
Migrated C# ModuleDef to V9 as well as adds tests to ensure that identical modules indeed produce identical schemas. The two remaining discrepancies between Rust and C# are: - #1891 - Row-level security not having a designed API & implementation for C#. - Fixes #1948. - Fixes #1589. - Supersedes #1619.
Pull request was closed
Migrated C# ModuleDef to V9 as well as adds tests to ensure that identical modules indeed produce identical schemas. The two remaining discrepancies between Rust and C# are: - #1891 - Row-level security not having a designed API & implementation for C#. - Fixes #1948. - Fixes #1589. - Supersedes #1619.
Migrated C# ModuleDef to V9 as well as adds tests to ensure that identical modules indeed produce identical schemas. The two remaining discrepancies between Rust and C# are: - #1891 - Row-level security not having a designed API & implementation for C#. - Fixes #1948. - Fixes #1589. - Supersedes #1619.
Description of Changes
This provides snapshot testing for ModuleDef returned from Wasm modules. It will automatically compare ModuleDefs of test modules against both the previous snapshots, and across different languages.
Note that comparison doesn't (can't) use raw ModuleDef. There are some minor non-functional differences between Rust and C# that need to be cleaned up before comparing - most notably, the order in which types are registered during table traverse, plus some smaller things like Rust registering column constraints even where they're no-op. I believe those changes don't matter and are fine to remove before comparing.
The history is split into two commits - one that adds working testing with all the cleanups, and in a separate one I added more compact debug representations for AlgebraicType and child types. That commit could be reverted, but it makes the snapshots much easier to read (see the diff) and I believe will be useful for regular debugging outside this test as well.
Fixes #1589.
API and ABI breaking changes
If this is an API or ABI breaking change, please apply the
corresponding GitHub label.
Expected complexity level and risk
How complicated do you think these changes are? Grade on a scale from 1 to 5,
where 1 is a trivial change, and 5 is a deep-reaching and complex change.
This complexity rating applies not only to the complexity apparent in the diff,
but also to its interactions with existing and future code.
If you answered more than a 2, explain what is complex about the PR,
and what other components it interacts with in potentially concerning ways.
Testing
Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!
cargo test -p spacetimedb-sdk).