Skip to content

Conversation

@RReverser
Copy link
Contributor

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!

  • Ran SDK tests (cargo test -p spacetimedb-sdk).
  • Write a test you want a reviewer to do here, so they can check it off when they're satisfied.

@RReverser RReverser requested review from gefjon and kazimuth August 20, 2024 18:39
@kazimuth
Copy link
Contributor

This is great, but possibly should be done against the new ModuleDef type from #1572 and #1606? Those are going to merge this week, they have been a little delayed because I kept getting blocked on other PRs merging. I would really love to implement pretty-printing for that ModuleDef.

@RReverser
Copy link
Contributor Author

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,
Copy link
Contributor Author

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?

Copy link
Contributor

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.

@bfops bfops added the release-any To be landed in any release window label Aug 26, 2024
@RReverser
Copy link
Contributor Author

RReverser commented Sep 4, 2024

@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 spacetime generate and extract_descriptions which we verify here still uses the V8 module definition, I think we should merge this as-is first and upgrade the test only after generation for V9 is implemented, otherwise we won't be comparing the same structures that generate uses.

Meanwhile, we'll at least already have some testing in place.

Copy link
Contributor

@kazimuth kazimuth left a 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.

RReverser added a commit that referenced this pull request Nov 28, 2024
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.
@RReverser RReverser closed this Nov 28, 2024
auto-merge was automatically disabled November 28, 2024 14:31

Pull request was closed

RReverser added a commit that referenced this pull request Nov 28, 2024
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.
RReverser added a commit that referenced this pull request Dec 9, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-any To be landed in any release window

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test coverage - assert that equivalent Rust and C# modules produce the same DescribeModule output

4 participants