-
Notifications
You must be signed in to change notification settings - Fork 23
Refactor/replace tryorama with sweettest #544
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
WalkthroughAdd SweetTest-based Rust test templates and helpers; inject holochain as a workspace dependency (with default-features = false) via a new workspace helper; bump multiple Holochain-related version constants to dev releases; update dev environment Node version and web-app test scripts; expose an API to add workspace dependencies; pass integrity zome manifests into entry-type templating; and add many template/unit tests and test helpers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
0740fd7 to
b4a2be5
Compare
b4a2be5 to
3caa96d
Compare
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.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/scaffold/zome/coordinator.rs`:
- Line 43: The Cargo dependency declaration for the holochain crate currently
includes a non-existent feature "transport-iroh"; remove "transport-iroh" from
the features list on the holochain dependency (the line containing holochain =
{{ workspace = true, optional = true, features = ["wasmer_sys",
"transport-iroh", "test_utils"] }}) or, if that transport feature is needed, add
a separate dependency entry for holochain_cli_client and put "transport-iroh" in
its features instead; ensure the holochain features list only contains valid
features such as "wasmer_sys" and "test_utils".
In `@src/templates/entry_type/tests/entry_type.rs`:
- Around line 362-385: The function expected_rendered_create_and_update
currently hardcodes the origin getter name ("get_original_test_post") when
calling expected_create_and_read_test; change this to build the name from the
entry_type_name_snake_case parameter (e.g., using
format!("get_original_{entry_type_name_snake_case}")) so it matches
expected_rendered_create_and_update_and_delete and works for non-"test_post"
types; update the call site in expected_rendered_create_and_update (and ensure
the second argument remains "hash.clone()" and the third stays None) to use the
dynamic name.
In
`@templates/generic/entry-type/dnas/`{{dna_role_name}}/zomes/coordinator/{{coordinator_zome_manifest.name}}/tests/common.rs.hbs:
- Around line 16-25: When cardinality is "vector" and the template emits a
linked_from value for {{field_name}}, it's producing a single hash instead of a
Vec; update the branch in the {{coordinator_zome_manifest.name}} test template
so that when {{`#if` (eq cardinality "vector")}} and the linked_from is not
self-referential you wrap the produced hash in a Vec (e.g. vec![<hash>]) —
preserve the existing distinction between ActionHash (signed_action.hashed.hash)
and entry hash (signed_action.hashed.content.entry_hash()) but wrap either
result in vec![...] so {{field_name}} gets a Vec of hashes rather than a single
hash.
🧹 Nitpick comments (2)
templates/generic/collection/dnas/{{dna_role_name}}/zomes/coordinator/{{coordinator_zome_manifest.name}}/tests/{{kebab_case collection_name}}.rs.hbs (1)
16-21: Verify cell ordering assumption incells_flattened().The test assumes
cells[0]belongs to the first conductor andcells[1]to the second. While this typically holds forSweetConductorBatch::standard(n)with a single DNA, the ordering relies on internal implementation details. Consider adding a brief comment documenting this assumption for future maintainability.src/scaffold/app/cargo.rs (1)
235-238: The#[inline]attribute is unnecessary here.For a trivial function that only returns
PathBuf::new().join("Cargo.toml"), the compiler will inline this automatically. The#[inline]attribute is typically only needed for cross-crate inlining of non-trivial functions.♻️ Suggested simplification
-#[inline] fn workspace_cargo_toml_path(_app_file_tree: &FileTree) -> PathBuf { PathBuf::new().join("Cargo.toml") }
...s/{{dna_role_name}}/zomes/coordinator/{{coordinator_zome_manifest.name}}/tests/common.rs.hbs
Outdated
Show resolved
Hide resolved
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@templates/generic/entry-type/dnas/`{{dna_role_name}}/zomes/coordinator/{{coordinator_zome_manifest.name}}/tests/common.rs.hbs:
- Around line 20-34: The template calls create_{{snake_case
linked_from.name}}(...).await.signed_action.hashed.content.entry_hash() for
fields that expect an EntryHash but doesn't handle the Option; update those
occurrences (the branches that call entry_hash()) to call
.entry_hash().unwrap().clone() so the value matches the expected EntryHash type
(keep the ActionHash branches unchanged); locate the calls within the
coordinator zome test template around the linked_from cases and replace
.entry_hash() with .entry_hash().unwrap().clone().
...s/{{dna_role_name}}/zomes/coordinator/{{coordinator_zome_manifest.name}}/tests/common.rs.hbs
Outdated
Show resolved
Hide resolved
...s/coordinator/{{coordinator_zome_manifest.name}}/tests/{{kebab_case collection_name}}.rs.hbs
Show resolved
Hide resolved
...s/{{dna_role_name}}/zomes/coordinator/{{coordinator_zome_manifest.name}}/tests/common.rs.hbs
Show resolved
Hide resolved
...s/coordinator/{{coordinator_zome_manifest.name}}/tests/{{kebab_case entry_type.name}}.rs.hbs
Show resolved
Hide resolved
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/templates/entry_type/tests/common.rs`:
- Around line 297-460: The generated struct field type doesn't become optional
for self-referential linked_from fields, causing a mismatch with templates that
render None; update FieldDefinition::rust_type() to detect when linked_from is
Some(Referenceable::EntryType(entry_ref)) and entry_ref.entry_type equals the
parent EntryDefinition.name and cardinality is Cardinality::Single, and in that
case return an Option-wrapped type (as if Cardinality::Option) instead of the
raw ActionHash (or other type); ensure this logic references FieldDefinition,
rust_type(), Cardinality::Single, linked_from, and EntryTypeReference so
self-referential single fields are rendered as Option<T> to match the None
placeholder.
🧹 Nitpick comments (1)
src/templates/entry_type/tests/common.rs (1)
462-494: Consider a small helper or table-driven pattern to reduce repetition.
Most tests repeat the same EntryDefinition boilerplate; a simple builder helper would make adding new field types faster.
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/templates/entry_type/tests/common.rs`:
- Around line 585-606: The test option_linked_from_agent_pub_key sets
linked_from to Referenceable::Agent but keeps field_type as FieldType::String,
causing a mismatch with the template which uses linked_from.hash_type(); update
the FieldDefinition in the EntryDefinition within the
option_linked_from_agent_pub_key test to use field_type: FieldType::AgentPubKey
so the rendered output and test expectation align with the linked_from-derived
type.
🧹 Nitpick comments (1)
templates/generic/entry-type/dnas/{{dna_role_name}}/zomes/coordinator/{{coordinator_zome_manifest.name}}/tests/common.rs.hbs (1)
11-101: Consider extracting deeply nested conditionals into partial templates.The
sample_{{snake_case entry_type.name}}function has 8+ levels of nesting due to the combinatorial logic forlinked_from,cardinality, andfield_type. While functionally correct, this depth makes the template harder to maintain and debug.If Handlebars partials are available in your template system, consider extracting logical blocks (e.g.,
linked_from_vector.hbs,no_linked_from_single.hbs) to improve readability.
...s/coordinator/{{coordinator_zome_manifest.name}}/tests/{{kebab_case collection_name}}.rs.hbs
Show resolved
Hide resolved
mattyg
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.
Monumental. I reviewed the best I could and scaffolded an app. Thanks for adding so many tests.
35f7c9a to
192cd58
Compare
|
✔️ 192cd58 - Conventional commits check succeeded. |
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/templates/entry_type/tests/common.rs`:
- Around line 630-651: The test vector_action_hash_linked_from_self_reference
defines an invalid self-referential linked_from for a Vector cardinality; remove
or update this test so it only covers allowed self-references
(Cardinality::Option) — locate the test function
vector_action_hash_linked_from_self_reference and either delete it or change the
FieldDefinition so cardinality is Cardinality::Option (or remove the
linked_from), keeping the rest of the parameters (EntryDefinition,
FieldType::ActionHash, Referenceable::EntryType / EntryTypeReference) unchanged
to produce a valid scenario.
🧹 Nitpick comments (1)
run_test.sh (1)
2-2: Gateset -xto avoid leaking secrets in logs.
Consider enabling tracing only when explicitly requested.🔧 Suggested change
-set -xe +set -e +[[ -n "${TRACE:-}" ]] && set -x
This PR adds Rust tests when an entry type or a collection are scaffolded. It also adds unit tests for some of the used functions.
It was a nightmare to fiddle with Rust's idiosyncrasies like ownership that requires clones, strong typing etc. and all that having to always rebuild the whole binary to pick up the changed templates and then run through scaffolding a web-app and entries to see the changes in rendered files. Therefore the unit tests. Which are a bit easier to handle, but the repo structure is still pretty clunky.
Note that I haven't deleted anything Tryorama related yet, to make this PR easier to review.
resolves #533
Summary by CodeRabbit
New Features
Tests
Chores