Skip to content

Conversation

@gefjon
Copy link
Contributor

@gefjon gefjon commented Aug 31, 2023

Description of Changes

Draft; PR currently contains first-draft harness and test module, but no tests for the Rust SDK.

API and ABI

  • This is a breaking change to the module ABI
  • This is a breaking change to the module API
  • This is a breaking change to the ClientAPI
  • This is a breaking change to the SDK API

If the API is breaking, please state below what will break

Yet to come: actual SDK tests.
Compiling the module_bindings for the sdk-test module revealed two bugs:

- Enums holding structs generated incorrectly,
  unpacking the struct into the enum's payload.

- Recursive types would cause the codegen to attempt to recursively import
  the current module into itself.
The sdk tests already build this crate (though they don't clippy or fmt it).

Attempting to build, test, fmt or clippy it as-is will fail
because the module_bindings are not committed.
This is intentional, as the SDK test suite wants to generate the module_bindings
during its run.
It turns out `cargo fmt` doesn't actually support the `--exclude` option
the way `cargo clippy` does.

Instead, just `#[rustfmt::skip]` the `mod module_bindings;` decl.
God, I'm so bad at remembering to commit new files.

Anyway, add a test for deleting rows with primitive unique fields.
The SDK tests need to run `spacetime start`, `spacetime generate` and `spacetime publish`.
I misread `Condvar::wait_timeout_while` as `Condvar::wait_timeout_until`,
and flipped my predicate.
This led to false negatives (i.e. tests that passed that shouldn't have).
This commit fixes a race condition which sometimes caused the SDK tests to fail
because multiple `spacetime generate` processes running concurrently
would clobber each others' output,
potentially deleting it while a `cargo build` or `cargo run` process was running.

Now, the test harness will only run `spacetime generate` at most once
for any given directory.
.nth(1)
.expect("Pass a test name as a command-line argument to the test client");

match &*test {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might make it more managable to break these tests out into separate files, but also fine to keep as a monolith if you feel its preferable 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we'd win much, if anything, from breaking the tests into separate files within a single test-client crate. I think it would be ideal to break the tests into a large number of module/client-project pairs (which is why I wrote the test harness to support that), but that would lead to quite a bit of crate proliferation in our workspace - in addition to 15 client crates and probably 12 or 13 modules, we'd need a shared utils crate for the modules, and another for the clients. If you think that's an improvement, I'm happy to do it (it would also help with parallelizing the tests more), but I wasn't sure about doing it of my own volition.

// having `Test::run` copy the `client_project` into a fresh temporary directory.
// That would be more complicated, as we'd need to re-write dependencies
// on the client language's SpacetimeDB SDK to use a local absolute path.
// Doing so portably across all our SDK languages seemed infeasible.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is fine for now, as we continue to add tests I think we will likely run into a similar problem that we ran into with the smoketests where it starts getting extremely slow running the tests in serial rather than parallel. I think as a first pass this decision is fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't run the actual tests in serial; we only run spacetime generate in serial, and that we only impose on tests that use the same client project. Cargo also imposes some linearity when it comes to compiling the module, and in the Rust SDK, the client project, but again, only for tests that use the same module or the same client project.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok this is fine then, thank you for the response 👍

Copy link
Collaborator

@jdetter jdetter left a comment

Choose a reason for hiding this comment

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

Phobe gave excellent responses to my questions on this, I think its well implemented and as a first pass this is more than acceptable. Awesome job Phoebe 👍

@jdetter jdetter merged commit 32ac808 into master Sep 12, 2023
bfops added a commit that referenced this pull request Jul 17, 2025
…ument. (#258)

# Description of Changes
Update the C# server and client SDK quickstart-chat example to match the
code presented in the tutorial, as of
clockworklabs/spacetime-docs#170 .
Also renamed the directory from `quickstart` to `quickstart-chat` in
order to be more specific.

# 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

1

## Requires SpacetimeDB PRs
SpacetimeDB branch name: master
com.clockworklabs.spacetimedbsdk: master

# 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!*

- [x] Ran `quickstart-chat` C# server and C# client locally and:
  - Set my name.
  - Sent a message.
  - Restarted and viewed the message backlog.
  - Sent a few more messages.
- Exited, deleted my local token, restarted and connected as a new
identity.
  - Set my new identity's name.
  - Sent a message as my new identity.

---------

Co-authored-by: Zeke Foppa <[email protected]>
Co-authored-by: John Detter <[email protected]>
Co-authored-by: james gilles <[email protected]>
bfops added a commit that referenced this pull request Aug 7, 2025
…ument. (#258)

# Description of Changes
Update the C# server and client SDK quickstart-chat example to match the
code presented in the tutorial, as of
clockworklabs/spacetime-docs#170 .
Also renamed the directory from `quickstart` to `quickstart-chat` in
order to be more specific.

# 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

1

## Requires SpacetimeDB PRs
SpacetimeDB branch name: master
com.clockworklabs.spacetimedbsdk: master

# 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!*

- [x] Ran `quickstart-chat` C# server and C# client locally and:
  - Set my name.
  - Sent a message.
  - Restarted and viewed the message backlog.
  - Sent a few more messages.
- Exited, deleted my local token, restarted and connected as a new
identity.
  - Set my new identity's name.
  - Sent a message as my new identity.

---------

Co-authored-by: Zeke Foppa <[email protected]>
Co-authored-by: John Detter <[email protected]>
Co-authored-by: james gilles <[email protected]>
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.

3 participants