From 699fe60ba50c94cf024a873fe57dc51085194f35 Mon Sep 17 00:00:00 2001 From: Joe Date: Sat, 14 Feb 2026 16:29:02 -0600 Subject: [PATCH] Add integration tests for Ref properties in standalone file formats - Introduced a new audit plan addressing test coverage gaps identified in the fourth audit of the two-way sync Ref property feature. - Added integration tests for Ref properties on standalone scripts and non-script standalone files, ensuring correct handling of adjacent meta file paths. - Created a new fixture file for testing purposes, enhancing the robustness of the test suite. These changes improve test coverage and reliability of the synchronization system's handling of Ref properties. --- .../ref_audit_round_4_test_coverage.plan.md | 216 ++++++++++++++++++ .../src/Workspace/TestString.txt | 1 + tests/tests/two_way_sync.rs | 94 ++++++++ 3 files changed, 311 insertions(+) create mode 100644 .cursor/plans/ref_audit_round_4_test_coverage.plan.md create mode 100644 rojo-test/serve-tests/ref_two_way_sync/src/Workspace/TestString.txt diff --git a/.cursor/plans/ref_audit_round_4_test_coverage.plan.md b/.cursor/plans/ref_audit_round_4_test_coverage.plan.md new file mode 100644 index 00000000..789de614 --- /dev/null +++ b/.cursor/plans/ref_audit_round_4_test_coverage.plan.md @@ -0,0 +1,216 @@ +--- +name: "" +overview: "" +todos: [] +isProject: false +--- + +# Fix Ref Two-Way Sync Audit Round 4: Test Coverage Gaps + +> This plan was generated by `/audit` from an Agent-mode session. +> Refer to `.cursor/rules/atlas.mdc` for full project standards. + +## Standards + +These standards govern every fix in this plan. An implementer MUST read `.cursor/rules/atlas.mdc` before starting. + +### Round-Trip Identity (from `atlas.mdc` Quality Standard) + +Syncback (or two-way sync) writes a directory tree. Building an rbxl from that directory tree and forward-syncing it back must produce a **bit-identical instance tree** -- same names, same classes, same properties, same hierarchy, same ref targets. Any deviation is a bug. + +### CLI Syncback Parity (from `atlas.mdc` Two-Way Sync Strategy) + +Plugin-based sync must produce **exactly the same filesystem output** that `atlas syncback` would give for the same input. Byte-for-byte identical files, identical directory structures, identical naming. Any divergence is a bug until proven otherwise. + +### Code Quality (from `atlas.mdc` Code Quality Standard) + +DRY code that is easy to reason about and hard to get wrong. Small refactors (2-3 call sites) are included as fixes. Major rewrites (10+ call sites) are deferred. + +## Context + +Fourth audit of the two-way sync Ref property feature (branch `refs` vs `origin/master`). Previous audits found and fixed: ServeSession missing instanceMap, stale paths after rename, slash escaping, priority conflict, encodeInstance leak, filter_properties_for_meta, slugified name fallback, immediate ID assignment, RefPathIndex with startup population, precise removal, and slugified path remapping. + +**This audit found NO critical issues and NO correctness bugs.** The implementation correctly handles all round-trip identity requirements across all code paths. All 3 previous audit rounds' findings have been properly addressed. + +Two minor test coverage gaps were identified: the script-adjacent meta format and the non-script standalone file format (branches 2 and 4 of `syncback_updated_properties`) lack explicit integration tests for Ref properties. + +## Fixes + +### Fix 1: Integration Test for Ref on Standalone Script (Adjacent Meta Format) + +- **Status:** Approved +- **Finding:** Step 11a: The 4 File Format Branches -- branch 2 (standalone script with adjacent `ScriptName.meta.json5`) is not explicitly tested for Ref properties. +- **Files:** `tests/tests/two_way_sync.rs` +- **Problem:** The `syncback_updated_properties` code for standalone scripts strips `.server`/`.client`/`.plugin`/`.local`/`.legacy` suffixes from the file stem to compute the adjacent meta path. For example, `TestScript.server.luau` produces `TestScript.meta.json5`. A regression in suffix stripping would write the Rojo_Ref_* attribute to the wrong file (e.g., `TestScript.server.meta.json5`), causing forward-sync to not find it. +- **Solution:** Add an integration test `ref_on_standalone_script` that: + 1. Uses the existing `ref_two_way_sync` fixture (which already has `TestScript.server.luau`) + 2. Finds the `TestScript` instance via the API + 3. Sends a Ref property update (any Ref, e.g., PrimaryPart pointing to Part1 -- scripts can have arbitrary Ref properties via Rojo_Ref_* attributes even if the class doesn't define them) + 4. Verifies `TestScript.meta.json5` contains the Rojo_Ref_* attribute (NOT `TestScript.server.meta.json5`) + The `ref_test_setup` helper already finds Part1 and other instances. The test just needs to also find the `TestScript` instance. Update `ref_test_setup` to return `script_id` as well, or find it inline. + ```rust + #[test] + fn ref_on_standalone_script() { + run_serve_test("ref_two_way_sync", |session, _| { + let (session_id, _, _, part1_id, _, _) = ref_test_setup(&session); + + // Find TestScript + let info = session.get_api_rojo().unwrap(); + let root_read = session.get_api_read(info.root_instance_id).unwrap(); + let (workspace_id, _) = find_by_name(&root_read.instances, "Workspace"); + let ws_read = session.get_api_read(workspace_id).unwrap(); + let (script_id, _) = find_by_name(&ws_read.instances, "TestScript"); + + let mut props = UstrMap::default(); + props.insert(ustr("PrimaryPart"), Some(Variant::Ref(part1_id))); + send_update( + &session, + &session_id, + InstanceUpdate { + id: script_id, + changed_name: None, + changed_class_name: None, + changed_properties: props, + changed_metadata: None, + }, + ); + + // Adjacent meta should be ScriptName.meta.json5 (NOT ScriptName.server.meta.json5) + let meta_path = session + .path() + .join("src/Workspace/TestScript.meta.json5"); + poll_meta_has_ref_attr( + &meta_path, + "Rojo_Ref_PrimaryPart", + "Workspace/TestModel/Part1", + ); + + // Verify the wrong path does NOT have the attribute + let wrong_meta = session + .path() + .join("src/Workspace/TestScript.server.meta.json5"); + assert!( + !wrong_meta.exists(), + "TestScript.server.meta.json5 should NOT be created" + ); + }); + } + ``` +- **Risk:** None -- test-only addition. +- **Verify round-trip:** The test itself verifies the correct meta file path. Forward-sync would read `TestScript.meta.json5` alongside `TestScript.server.luau`, which is the existing behavior for adjacent meta. +- **Verify syncback parity:** N/A -- this tests the two-way sync write path only. +- **Tests required:** + - The fix IS the test. + +--- + +### Fix 2: Integration Test for Ref on Non-Script Standalone File (Adjacent Meta Format) + +- **Status:** Approved +- **Finding:** Step 11a: The 4 File Format Branches -- branch 4 (non-script standalone file like `.txt` with adjacent `FileName.meta.json5`) is not explicitly tested for Ref properties. +- **Files:** `tests/tests/two_way_sync.rs`, `rojo-test/serve-tests/ref_two_way_sync/src/Workspace/TestString.txt` +- **Problem:** The `syncback_updated_properties` code for non-script standalone files computes the adjacent meta path as `file_stem + .meta.json5`. This is the simplest branch (no suffix stripping), but it's the only branch with zero test coverage for Ref properties. +- **Solution:** + 1. Add a fixture file `rojo-test/serve-tests/ref_two_way_sync/src/Workspace/TestString.txt` containing any text (e.g., `"hello"`). This creates a `StringValue` instance named `TestString`. + 2. Add an integration test `ref_on_txt_file_instance` that: + - Finds the `TestString` instance via the API + - Sends a Ref property update + - Verifies `TestString.meta.json5` contains the Rojo_Ref_* attribute + ```rust + #[test] + fn ref_on_txt_file_instance() { + run_serve_test("ref_two_way_sync", |session, _| { + let (session_id, _, _, part1_id, _, _) = ref_test_setup(&session); + + let info = session.get_api_rojo().unwrap(); + let root_read = session.get_api_read(info.root_instance_id).unwrap(); + let (workspace_id, _) = find_by_name(&root_read.instances, "Workspace"); + let ws_read = session.get_api_read(workspace_id).unwrap(); + let (string_id, _) = find_by_name(&ws_read.instances, "TestString"); + + let mut props = UstrMap::default(); + props.insert(ustr("PrimaryPart"), Some(Variant::Ref(part1_id))); + send_update( + &session, + &session_id, + InstanceUpdate { + id: string_id, + changed_name: None, + changed_class_name: None, + changed_properties: props, + changed_metadata: None, + }, + ); + + // Adjacent meta for .txt is FileName.meta.json5 + let meta_path = session + .path() + .join("src/Workspace/TestString.meta.json5"); + poll_meta_has_ref_attr( + &meta_path, + "Rojo_Ref_PrimaryPart", + "Workspace/TestModel/Part1", + ); + }); + } + ``` +- **Risk:** None -- test-only addition plus one tiny fixture file. +- **Verify round-trip:** The test verifies the correct meta file path. Forward-sync reads `TestString.meta.json5` alongside `TestString.txt`, which is existing behavior. +- **Verify syncback parity:** N/A +- **Tests required:** + - The fix IS the test, plus the fixture file. + +--- + +## Skipped Fixes + +None. All findings were approved. + +## Accepted Limitations + +- **Ambiguous paths (duplicate-named siblings):** Properly warned via `is_ref_path_unique`. Ref is written but may resolve to wrong sibling on rebuild. Accepted in all four audits. Warning is logged, no silent corruption beyond documented scope. +- **Transient stale path window during rename:** Between directory rename on disk and meta file rewrite by `update_ref_paths_after_rename`, there is a millisecond-wide window where the meta file contains a stale Rojo_Ref_* path. A server crash in this window would leave the path stale until next server startup (which runs `reconcile_tree` to self-correct). Inherent to non-atomic multi-file operations. Accepted after evaluating practical alternatives (self-healing on startup, reverse operation order, full journaling -- all either insufficient or disproportionately complex). + +## Deferred Refactors + +- **ChangeProcessor::start parameter count (8/7):** Clippy warns about too many parameters. Consider grouping into a config struct. Pre-existing issue worsened by adding `ref_path_index`. Flagged in audit round 3. + +## Test Plan + +Tests are not optional. Every approved fix must have test coverage. A fix is not complete until its tests pass. + +### Rust Integration Tests (`tests/tests/two_way_sync.rs`) + +- `**ref_on_standalone_script**` (Fix 1): Set Ref on script instance, verify `TestScript.meta.json5` (not `TestScript.server.meta.json5`) gets the Rojo_Ref_* attribute. +- `**ref_on_txt_file_instance**` (Fix 2): Set Ref on StringValue (.txt) instance, verify `TestString.meta.json5` gets the Rojo_Ref_* attribute. + +### Fixture Files + +- `**rojo-test/serve-tests/ref_two_way_sync/src/Workspace/TestString.txt**` (Fix 2): New fixture file containing `"hello"` to create a StringValue instance. + +### No New Tests Required For + +- Fix 1: Uses existing `TestScript.server.luau` fixture. + +## Test Rules + +The implementer MUST follow these rules when writing tests for this plan: + +### For missing coverage identified + +For each "missing test coverage" gap: + +1. Write the test +2. Verify it passes against the current implementation +3. If it fails, investigate -- it may have found another bug + +### Scope: test every affected layer + +For each finding, determine which test layers are affected and write tests in ALL of them: + +- Does the bug manifest at the server level? -> Rust integration test with `/api/write` + filesystem verification +- Could it regress from a different code path? -> Test that code path too + +## Final Step: Run CI + +After ALL fixes and tests are implemented, run the `/ci` command (see `.cursor/commands/ci.md`) to execute the full CI pipeline. Every fix must pass CI before the plan is considered complete. Do not skip this step. \ No newline at end of file diff --git a/rojo-test/serve-tests/ref_two_way_sync/src/Workspace/TestString.txt b/rojo-test/serve-tests/ref_two_way_sync/src/Workspace/TestString.txt new file mode 100644 index 00000000..b6fc4c62 --- /dev/null +++ b/rojo-test/serve-tests/ref_two_way_sync/src/Workspace/TestString.txt @@ -0,0 +1 @@ +hello \ No newline at end of file diff --git a/tests/tests/two_way_sync.rs b/tests/tests/two_way_sync.rs index cb96b19d..4142574e 100644 --- a/tests/tests/two_way_sync.rs +++ b/tests/tests/two_way_sync.rs @@ -7101,3 +7101,97 @@ fn ref_rename_to_slugified_name_updates_ref_paths() { ); }); } + +// =========================================================================== +// Ref on different standalone file formats (meta path computation coverage) +// +// These tests cover branches 2 and 4 of syncback_updated_properties: +// - Branch 2: Standalone script → adjacent ScriptName.meta.json5 +// (strips .server/.client/.plugin/.local/.legacy suffix from stem) +// - Branch 4: Non-script standalone file (.txt, .csv) → adjacent FileName.meta.json5 +// =========================================================================== + +/// Setting a Ref property on a standalone script should write to the adjacent +/// meta file with the compound suffix stripped: TestScript.server.luau → +/// TestScript.meta.json5 (NOT TestScript.server.meta.json5). +#[test] +fn ref_on_standalone_script() { + run_serve_test("ref_two_way_sync", |session, _| { + let (session_id, _, _, part1_id, _, _) = ref_test_setup(&session); + + // Find TestScript + let info = session.get_api_rojo().unwrap(); + let root_read = session.get_api_read(info.root_instance_id).unwrap(); + let (workspace_id, _) = find_by_name(&root_read.instances, "Workspace"); + let ws_read = session.get_api_read(workspace_id).unwrap(); + let (script_id, _) = find_by_name(&ws_read.instances, "TestScript"); + + let mut props = UstrMap::default(); + props.insert(ustr("PrimaryPart"), Some(Variant::Ref(part1_id))); + send_update( + &session, + &session_id, + InstanceUpdate { + id: script_id, + changed_name: None, + changed_class_name: None, + changed_properties: props, + changed_metadata: None, + }, + ); + + // Adjacent meta should be ScriptName.meta.json5 (NOT ScriptName.server.meta.json5) + let meta_path = session.path().join("src/Workspace/TestScript.meta.json5"); + poll_meta_has_ref_attr( + &meta_path, + "Rojo_Ref_PrimaryPart", + "Workspace/TestModel/Part1", + ); + + // Verify the wrong path does NOT have the attribute + let wrong_meta = session + .path() + .join("src/Workspace/TestScript.server.meta.json5"); + assert!( + !wrong_meta.exists(), + "TestScript.server.meta.json5 should NOT be created" + ); + }); +} + +/// Setting a Ref property on a non-script standalone file (.txt → StringValue) +/// should write to the adjacent FileName.meta.json5. +#[test] +fn ref_on_txt_file_instance() { + run_serve_test("ref_two_way_sync", |session, _| { + let (session_id, _, _, part1_id, _, _) = ref_test_setup(&session); + + let info = session.get_api_rojo().unwrap(); + let root_read = session.get_api_read(info.root_instance_id).unwrap(); + let (workspace_id, _) = find_by_name(&root_read.instances, "Workspace"); + let ws_read = session.get_api_read(workspace_id).unwrap(); + let (string_id, _) = find_by_name(&ws_read.instances, "TestString"); + + let mut props = UstrMap::default(); + props.insert(ustr("PrimaryPart"), Some(Variant::Ref(part1_id))); + send_update( + &session, + &session_id, + InstanceUpdate { + id: string_id, + changed_name: None, + changed_class_name: None, + changed_properties: props, + changed_metadata: None, + }, + ); + + // Adjacent meta for .txt is FileName.meta.json5 + let meta_path = session.path().join("src/Workspace/TestString.meta.json5"); + poll_meta_has_ref_attr( + &meta_path, + "Rojo_Ref_PrimaryPart", + "Workspace/TestModel/Part1", + ); + }); +}