Expansion Source Tests

[kans]

Summary by CodeRabbit

• Tests
  • Added a comprehensive correctness test for the expansion process that verifies grant counts, detects missing or extra grants, and compares per-grant source entitlements; emits diagnostic details for mismatches and ensures expanded data matches expected results.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds a new integration-style correctness test that runs the expander on unexpanded C1Z testdata, loads the expected expanded C1Z, and compares grant counts, presence, and per-grant source entitlement sets.

Changes

Cohort / File(s)	Change Summary
Test Correctness pkg/sync/expand/expand_correctness_test.go	New Go test TestExpandCorrectness plus helpers: locates testdata, copies unexpanded C1Z to temp, sets sync ID, runs expander, loads grants (paginated) from actual and expected C1Zs, compares counts, computes missing/extra grants, compares per-grant source sets, and reports diagnostics.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Review grant loading/pagination and grantKey composition.
• Check comparison logic for missing/extra grants and source-set diffs.
• Validate test setup/teardown (temp copy, SetSyncID) and short-mode/file-missing skips.

Possibly related PRs

• A couple of Grant Expansion Benchmarks #594 — Related test tooling and C1Z graph loading helpers used by the new test.
• Refactor Expansion for Testability  #585 — Adds Expander implementation that this test exercises.

Suggested reviewers

• ggreer

Poem

"I'm a rabbit in the codebase glade,
I copy files and watch grants parade,
I hop through sources, count each key,
If matches pass, I jig with glee,
Hooray — the expander's dance is made!" 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Expansion Source Tests' accurately reflects the main change: a new test file for validating expansion logic with focus on source comparison.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch kans/grant-expansion-comparison

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

pkg/sync/expand/expand_correctness_test.go (1)

149-150: Consider deferring count assertion for better diagnostics.

The require.Equal on line 150 will fail immediately if counts differ, preventing the more detailed missing/extra grant analysis below from running. Moving this assertion after the detailed analysis (near line 233-235) would provide better diagnostic output when the test fails.

-		// Compare grant counts
-		require.Equal(t, len(expectedGrants), len(actualGrants), "grant count mismatch")
+		t.Logf("Actual grants: %d, Expected grants: %d", len(actualGrants), len(expectedGrants))

Then consolidate the count check with the final assertions at lines 233-235.

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ac3ba04 and 5d746d6.

📒 Files selected for processing (1)

• pkg/sync/expand/expand_correctness_test.go (1 hunks)

🧰 Additional context used

🪛 GitHub Check: go-lint

pkg/sync/expand/expand_correctness_test.go

[failure] 71-71:
Comment should end in a period (godot)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: go-test (1.25.2, windows-latest)
• GitHub Check: go-test (1.25.2, ubuntu-latest)

🔇 Additional comments (3)

pkg/sync/expand/expand_correctness_test.go (3)

23-36: LGTM!

Good use of composite key for grant uniqueness and map-based set for sources.

---

49-58: LGTM, but note potential silent overwrite.

If two grants ever share the same grantKey (same entitlement + principal), the second one silently overwrites the first. This is likely fine given the uniqueness semantics of grants, but worth being aware of during debugging if grant counts ever mismatch unexpectedly.

---

191-235: LGTM - thorough comparison logic.

The source comparison correctly handles both count mismatches and individual source differences. The sorted output and limited logging (first 10/20 items) keeps failure messages manageable while still being informative.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

pkg/sync/expand/expand_correctness_test.go (1)

83-87: Consider uncommenting or removing the disabled test case.

The "SmallMedium" test case is commented out. If it's temporarily disabled, consider adding a comment explaining why, or use t.Skip() with a reason instead for better visibility.

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4ca1b61 and fec998f.

📒 Files selected for processing (1)

• pkg/sync/expand/expand_correctness_test.go (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

pkg/sync/expand/expand_correctness_test.go (3)

pkg/types/tasks/tasks.go (1)

• GetResourceType (92-92)

pkg/dotc1z/c1file.go (3)

• C1File (36-54)
• NewC1ZFile (174-210)
• WithReadOnly (158-162)

pkg/sync/expand/expander.go (1)

• NewExpander (42-47)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: go-test (1.25.2, windows-latest)

🔇 Additional comments (8)

pkg/sync/expand/expand_correctness_test.go (8)

18-21: LGTM!

Standard pattern for locating testdata relative to the test file's directory using runtime.Caller(0).

---

23-30: LGTM!

The key composition using entitlement ID and principal identifiers is appropriate for matching grants across different C1Z files.

---

32-36: LGTM!

Simple and effective data structure for holding grant information during comparison.

---

49-58: Potential grant overwrite if duplicates exist.

If two grants share the same grantKey (same entitlement and principal), the second grant will silently overwrite the first in the map. This could mask duplicate grant issues in the test data.

If grants are guaranteed unique by this key, this is fine. Otherwise, consider either detecting duplicates or accumulating them.

---

70-73: LGTM!

Good use of testing.Short() to skip this slow test during quick test runs.

---

104-114: LGTM!

The file copy pattern is correct: create temp file to obtain a unique path, close the handle, then write content via os.WriteFile. The deferred os.Remove ensures cleanup.

---

193-221: Source comparison could report both missing and extra sources for the same grant.

When source counts differ, the code reports the count mismatch and continues (line 205), skipping the detailed per-source comparison. However, if counts match but sources differ, both missing and extra sources can be appended for the same grant key. This is fine for diagnostics but could produce multiple entries per grant.

This behavior is acceptable for debugging purposes.

---

235-237: LGTM!

Using require.Empty after detailed t.Errorf output provides both rich diagnostics and clear pass/fail semantics.