test: close TOML/JSON fixture divergence for passthrough.exclude and watch#1129
Conversation
…watch (#1127) Add passthrough entries with exclude arrays and watch entries to valid.toml and valid.json to match valid.yaml. Expand TOML and JSON format-parity test contexts from 3 spot-checks to comprehensive assertions covering all config sections. Refs #1127 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for alloyssg ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughExpanded TOML and JSON configuration fixtures with passthrough and watch entries, and replaced coarse parity checks with detailed assertions covering parsed configuration fields. ChangesConfiguration format parity
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
zeroedin
left a comment
There was a problem hiding this comment.
Code Review Results
Scope: internal/config/config_test.go, testdata/valid.{toml,json} — 3 files, +290/−8
Intent: Close TOML/JSON fixture divergence for passthrough.exclude and watch; expand format-parity assertions from spot-checks to comprehensive coverage
P1 — High
| # | File | Issue | Reviewer | Confidence | Route |
|---|---|---|---|---|---|
| 1 | PR body | Closes #1127 will auto-close the issue on merge. This is a spec PR — per project rules, spec PRs must not auto-close issues. The commit message correctly uses (#1127) as a reference, but GitHub's keyword detection in the PR body overrides that. Change to Refs #1127. |
project-standards | 90 | Fix before merge |
| 2 | config_test.go:298-308, 440-450 |
TOML/JSON "parses sources identically to YAML" blocks assert only products.Type and products.Endpoint but drop products.Query, products.Cache, and products.As — all three are asserted in the YAML test (lines 171-173). Query is the graphql-specific field; if TOML/JSON parsers mangle it, nothing catches it. The test description claims full parity but delivers partial. |
testing, correctness | 90 | Fix before merge |
| 3 | config_test.go:340-350, 482-492 |
TOML/JSON "parses languages identically to YAML" blocks assert en.Strings["read_more"] but drop en.Strings["posted_on"], fr.Strings["read_more"], and fr.Strings["posted_on"] — all asserted in the YAML test (lines 215-221). The Strings map is the only nested-map-within-a-map structure in languages config; this is exactly the structure most likely to exhibit format-specific parsing differences. |
testing, correctness | 90 | Fix before merge |
P2 — Moderate
| # | File | Issue | Reviewer | Confidence | Route |
|---|---|---|---|---|---|
| 4 | config_test.go:334-338, 476-480 |
TOML/JSON "parses SSR config identically to YAML" blocks assert SSR.Command but omit SSR.Mode empty-string assertion present in the YAML test (line 204). If a TOML/JSON decoder populated Mode with a non-empty default, the parity tests would not catch it. |
testing | 80 | Fix before merge |
P3 — Advisory
| # | File | Issue | Reviewer | Confidence | Route |
|---|---|---|---|---|---|
| 5 | config_test.go:225-507 |
The TOML and JSON Context blocks are ~140 lines each with near-identical structure — same 14 It blocks, same assertions, differing only in variable name and format string. Findings #2-#4 above are a direct consequence: assertions were added to one format's reference (YAML) but not propagated to both parity blocks. A DescribeTable/Entry parameterized by format and fixture path would make divergence mechanically impossible. The file doesn't use DescribeTable elsewhere, so this is a style break — but the duplication cost is real and growing. |
maintainability | 75 | Consider |
Coverage
- Testing gaps: The three P1/P2 findings above represent 7 missing assertions per format (14 total) where the test descriptions claim "identically to YAML" but the assertions are a subset.
- Residual risks: Future config section additions require touching 6 places (3 fixtures + 3 test blocks). DescribeTable (P3 #5) would reduce this to 4.
What's clean
- Fixture values are byte-identical across all three formats — verified field-by-field
BeforeEachscoping is correct; eachItgets a freshconfig.LoadcallloadErrguard in everyItblock is defensive but justified (prevents nil-pointer panics)- Passthrough entries 2 and 3 (with
exclude) match exactly across YAML/TOML/JSON - Watch entries match exactly across all three formats
- Conventional Commits format ✅, no
encoding/json✅, noSkip()✅
Verdict: Ready with fixes
Reasoning: The fixture changes and new test structure are solid — the passthrough/exclude and watch parity gaps that motivated issue #1127 are fully closed. The PR falls short of its own "comprehensive coverage" claim by leaving 7 assertions per format that exist in the YAML reference tests but were not carried into the TOML/JSON blocks (sources/products fields, languages/strings entries, SSR mode). These are straightforward additions. The PR body auto-close keyword also needs a one-word edit.
Add missing parity assertions for TOML/JSON sources (products.Query, Cache, As), languages (posted_on, French strings), and SSR (Mode empty-string) to match the YAML reference tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Review responseP1 #1 — PR body auto-close keywordFixed. Changed P1 #2 — TOML/JSON sources missing products.Query, Cache, AsFixed in 0aa276a. Added P1 #3 — TOML/JSON languages missing posted_on and French stringsFixed in 0aa276a. Added P2 #4 — TOML/JSON SSR missing Mode empty-string assertionFixed in 0aa276a. Added P3 #5 — DescribeTable to reduce duplication (advisory)Skipped. The file doesn't use |
zeroedin
left a comment
There was a problem hiding this comment.
Code Review Results (Rereview)
Scope: internal/config/config_test.go, testdata/valid.{toml,json} — 3 files, +308/−8 (2 commits)
Intent: Close TOML/JSON fixture divergence for passthrough.exclude and watch; expand format-parity assertions
Reviewing fix commit 0aa276a against prior review findings on 254db89.
Prior findings — all resolved
| # | Prior finding | Status |
|---|---|---|
| 1 | P1: Closes #1127 auto-close keyword |
✅ PR body uses Refs #1127 — no auto-close keywords present |
| 2 | P1: Sources missing Query, Cache, As assertions |
✅ Added for both TOML (308-310) and JSON (459-461), matching YAML (171-173) |
| 3 | P1: Languages missing posted_on + French strings |
✅ Added for both TOML (352, 357-358) and JSON (503, 508-509), matching YAML (215, 220-221) |
| 4 | P2: SSR missing Mode empty-string assertion |
✅ Added for both TOML (341) and JSON (492), matching YAML (204) |
| 5 | P3: DescribeTable dedup advisory | Open — fine as a future refactor |
New findings
None. The fix commit is surgical — adds only the missing assertions, no regressions introduced. No encoding/json, no Skip(), conventional commits ✅.
Verdict: Ready
Reasoning: All blocking findings from the prior review are addressed. YAML/TOML/JSON parity assertions are now byte-identical across all three formats for every config section. The P3 DescribeTable suggestion remains valid as future cleanup but doesn't block.
Summary
passthroughentries (withexcludearrays) and 2watchentries tovalid.tomlandvalid.json, matching the existingvalid.yamlfixtureTitle,BaseURL,Build.Output, andStructure.ComponentsTest summary
It(4 assertions)ItblocksIt(4 assertions)ItblocksFixture changes
All 136 config specs pass (
go test -race ./internal/config/).Refs #1127
🤖 Generated with Claude Code
Summary by CodeRabbit
elementsbuild inputs into asset destinations, with exclusions for HTML, demo content, and minified files.elements(content) andshared-layouts(layout).