SDK test suites (#258)

* Simple SDK test harness; SDK test module

Yet to come: actual SDK tests.

* Quiet clippy lint about "too many arguments"

* Lints are named with underscores, not dashes...

* Will this make clippy shut up?

* Go nuclear on disabling `too_many_attributes` lint. Sigh.

* WIP SDK test client, and fix bugs in Rust codegen

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.

* One (1) actual runnable test in the sdk crate

* Exclude test-client from CI

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.

* Rustfmt ignore generated module_bindings

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.

* Actually commit test file...

God, I'm so bad at remembering to commit new files.

Anyway, add a test for deleting rows with primitive unique fields.

* Make CLI tool available in tests workflow

The SDK tests need to run `spacetime start`, `spacetime generate` and `spacetime publish`.

* Test update events with primitive pk types; split test-client into files

* Tests with `Identity` fields in tables

* Tests for reducer callbacks, both successful and failing

* Tests with vecs of stuff, with structs, with enums

* Test that should fail, test that uses a large table with many columns

* Test for resubscribe functionality

* Test of reauth; fix major bug in `TestCounter`

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).

* A fistful of doc comments

* Avoid race condition running multiple tests with same client project

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.

* Add env_logger to SDK test client

* RUST_LOG=trace when running test clients

* quieter logs in test client: only warn-level and higher

.github/workflows/ci.yml

@@ -30,13 +30,17 @@ jobs:
 
       - uses: dsherret/rust-toolchain-file@v1
 
+      - name: Install CLI tool
+        run: |
+          cargo install --path crates/cli
+
       - name: Create /stdb dir
         run: |
           sudo mkdir /stdb
           sudo chmod 777 /stdb
 
       - name: Run cargo test
-        run: cargo test --all --features odb_rocksdb,odb_sled
+        run: cargo test --all --exclude test-client --features odb_rocksdb,odb_sled
 
   lints:
     name: Lints
@@ -52,7 +56,7 @@ jobs:
         run: cargo fmt --all -- --check
 
       - name: Run cargo clippy
-        run: cargo clippy --all --tests --features odb_rocksdb,odb_sled -- -D warnings
+        run: cargo clippy --all --exclude test-client --tests --features odb_rocksdb,odb_sled -- -D warnings
 
   wasm_bindings:
     name: Build and test wasm bindings

Cargo.toml

@@ -19,6 +19,8 @@ members = [
   "modules/benchmarks",
   "modules/spacetimedb-quickstart",
   "modules/quickstart-chat",
+  "modules/sdk-test",
+  "crates/sdk/tests/test-client",
 ]
 default-members = ["crates/cli"]
 # cargo feature graph resolver. v2 is default in edition2021 but workspace
@@ -79,6 +81,7 @@ dirs = "5.0.1"
 duct = "0.13.5"
 email_address = "0.2.4"
 enum-as-inner = "0.6"
+env_logger = "0.10"
 flate2 = "1.0.24"
 fs2 = "0.4.3"
 fs-err = "2.9.0"

crates/cli/src/subcommands/generate/mod.rs

@@ -226,24 +226,20 @@ impl GenItem {
         match self {
             GenItem::Table(table) => {
                 let code = rust::autogen_rust_table(ctx, table);
-                let name = table.name.to_case(Case::Snake);
-                Some((name + ".rs", code))
+                Some((rust::rust_type_file_name(&table.name), code))
             }
             GenItem::TypeAlias(TypeAlias { name, ty }) => {
-                let filename = name.replace('.', "").to_case(Case::Snake);
-                let filename = filename + ".rs";
                 let code = match &ctx.typespace[*ty] {
                     AlgebraicType::Sum(sum) => rust::autogen_rust_sum(ctx, name, sum),
                     AlgebraicType::Product(prod) => rust::autogen_rust_tuple(ctx, name, prod),
                     _ => todo!(),
                 };
-                Some((filename, code))
+                Some((rust::rust_type_file_name(name), code))
             }
             GenItem::Reducer(reducer) if reducer.name == "__init__" => None,
             GenItem::Reducer(reducer) => {
                 let code = rust::autogen_rust_reducer(ctx, reducer);
-                let name = reducer.name.to_case(Case::Snake);
-                Some((name + "_reducer.rs", code))
+                Some((rust::rust_reducer_file_name(&reducer.name), code))
             }
         }
     }

crates/cli/src/subcommands/generate/rust.rs

@@ -199,10 +199,15 @@ pub fn autogen_rust_sum(ctx: &GenCtx, name: &str, sum_type: &SumType) -> String
 
     print_file_header(out);
 
+    // Pass this file into `gen_and_print_imports` to avoid recursively importing self
+    // for recursive types.
+    let file_name = name.to_case(Case::Snake);
+    let this_file = (file_name.as_str(), name);
+
     // For some reason, deref coercion doesn't work on `&sum_type.variants` here - rustc
     // wants to pass it as `&Vec<_>`, not `&[_]`. The slicing index `[..]` forces passing
     // as a slice.
-    gen_and_print_imports(ctx, out, &sum_type.variants[..], generate_imports_variants);
+    gen_and_print_imports(ctx, out, &sum_type.variants[..], generate_imports_variants, this_file);
 
     out.newline();
 
@@ -239,16 +244,6 @@ fn write_enum_variant(ctx: &GenCtx, out: &mut Indenter, variant: &SumTypeVariant
             // ```
             writeln!(out, ",").unwrap();
         }
-        AlgebraicType::Product(ProductType { elements }) => {
-            // If the contained type is a non-empty product, i.e. this variant is
-            // struct-like, write it with braces and named fields.
-            write_struct_type_fields_in_braces(
-                ctx, out, elements,
-                // Do not `pub`-qualify fields because enum fields are always public, and
-                // rustc errors on the redundant `pub`.
-                false,
-            );
-        }
         otherwise => {
             // If the contained type is not a product, i.e. this variant has a single
             // member, write it tuple-style, with parens.
@@ -352,6 +347,16 @@ pub fn autogen_rust_table(ctx: &GenCtx, table: &TableDef) -> String {
 //    - Complicated because `HashMap` is not `Hash`.
 // - others?
 
+pub fn rust_type_file_name(type_name: &str) -> String {
+    let filename = type_name.replace('.', "").to_case(Case::Snake);
+    filename + ".rs"
+}
+
+pub fn rust_reducer_file_name(type_name: &str) -> String {
+    let filename = type_name.replace('.', "").to_case(Case::Snake);
+    filename + "_reducer.rs"
+}
+
 const STRUCT_DERIVES: &[&str] = &["#[derive(Serialize, Deserialize, Clone, PartialEq, Debug)]"];
 
 fn print_struct_derives(output: &mut Indenter) {
@@ -363,7 +368,15 @@ fn begin_rust_struct_def_shared(ctx: &GenCtx, out: &mut Indenter, name: &str, el
 
     print_spacetimedb_imports(out);
 
-    gen_and_print_imports(ctx, out, elements, generate_imports_elements);
+    // Pass this file into `gen_and_print_imports` to avoid recursively importing self
+    // for recursive types.
+    //
+    // The file_name will be incorrect for reducer arg structs, but that doesn't matter
+    // because it's impossible for a reducer arg struct to be recursive.
+    let file_name = name.to_case(Case::Snake);
+    let this_file = (file_name.as_str(), name);
+
+    gen_and_print_imports(ctx, out, elements, generate_imports_elements, this_file);
 
     out.newline();
 
@@ -1099,17 +1112,35 @@ fn generate_imports(ctx: &GenCtx, imports: &mut Imports, ty: &AlgebraicType) {
     }
 }
 
-fn print_imports(out: &mut Indenter, imports: Imports) {
+/// Print `use super::` imports for each of the `imports`, except `this_file`.
+///
+/// `this_file` is passed and excluded for the case of recursive types:
+/// without it, the definition for a type like `struct Foo { foos: Vec<Foo> }`
+/// would attempt to include `import super::foo::Foo`, which fails to compile.
+fn print_imports(out: &mut Indenter, imports: Imports, this_file: (&str, &str)) {
     for (module_name, type_name) in imports {
-        writeln!(out, "use super::{}::{};", module_name, type_name).unwrap();
+        if (module_name.as_str(), type_name.as_str()) != this_file {
+            writeln!(out, "use super::{}::{};", module_name, type_name).unwrap();
+        }
     }
 }
 
-fn gen_and_print_imports<Roots, SearchFn>(ctx: &GenCtx, out: &mut Indenter, roots: Roots, search_fn: SearchFn)
-where
+/// Use `search_function` on `roots` to detect required imports, then print them with `print_imports`.
+///
+/// `this_file` is passed and excluded for the case of recursive types:
+/// without it, the definition for a type like `struct Foo { foos: Vec<Foo> }`
+/// would attempt to include `import super::foo::Foo`, which fails to compile.
+fn gen_and_print_imports<Roots, SearchFn>(
+    ctx: &GenCtx,
+    out: &mut Indenter,
+    roots: Roots,
+    search_fn: SearchFn,
+    this_file: (&str, &str),
+) where
     SearchFn: FnOnce(&GenCtx, &mut Imports, Roots),
 {
     let mut imports = HashSet::new();
     search_fn(ctx, &mut imports, roots);
-    print_imports(out, imports);
+
+    print_imports(out, imports, this_file);
 }

crates/sdk/Cargo.toml

@@ -27,7 +27,11 @@ tokio.workspace = true
 tokio-tungstenite.workspace = true
 
 [dev-dependencies]
+# for quickstart-chat and cursive-chat examples
 hex.workspace = true
 # for cursive-chat example
 cursive.workspace = true
 futures-channel.workspace = true
+
+# for tests
+spacetimedb-testing = { path = "../testing" }

crates/sdk/src/client_cache.rs

@@ -203,6 +203,12 @@ impl<T: TableType> TableCache<T> {
         );
 
         for (row_hash, row) in prev_subs.into_iter() {
+            log::trace!(
+                "Initalizing table {:?}: row previously resident: {:?} hash: {:?}",
+                T::TABLE_NAME,
+                row,
+                row_hash,
+            );
             if diff.insert(row_hash, DiffEntry::Delete(row)).is_some() {
                 // This should be impossible, but just in case...
                 log::error!("Found duplicate row in existing `TableCache` for {:?}", T::TABLE_NAME);
@@ -238,7 +244,12 @@ impl<T: TableType> TableCache<T> {
                         );
                     }
                     Ok(row) => {
-                        log::trace!("Initializing table {:?}: got new row {:?}", T::TABLE_NAME, row);
+                        log::trace!(
+                            "Initializing table {:?}: got new row {:?}. Hash: {:?}",
+                            T::TABLE_NAME,
+                            row,
+                            row_pk
+                        );
                         diff.insert(row_pk, DiffEntry::Insert(row));
                     }
                 },

crates/sdk/tests/test-client/Cargo.toml

@@ -0,0 +1,12 @@
+[package]
+name = "test-client"
+version = "0.1.0"
+edition = "2021"
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+
+[dependencies]
+spacetimedb-sdk = { path = "../.." }
+tokio.workspace = true
+anyhow.workspace = true
+env_logger.workspace = true

crates/sdk/tests/test-client/src/.gitignore

@@ -0,0 +1 @@
+module_bindings