spec(test): add tests for review items #160-#164

[zeroedin]

Summary

Adds spec tests for 5 review items from PR #153:

• spec(template): inline shortcode followed by more content may consume subsequent tokens #160 — Inline shortcode followed by more content must not consume subsequent tokens
• spec(template): test reverse filter without plugin override to verify nil fallthrough #161 — Built-in reverse filter nil fallthrough works without plugin override (split|reverse|join on arrays)
• feat(ssr): wire ssrCfg.Timeout to RenderPageWithTimeout in exec mode #162 — BuildPhase2 exec mode must enforce ssrCfg.Timeout (currently calls RenderPage without timeout)
• feat(plugin): bridge shortcodes from QuickJS to template engine in pipeline #163 — Pipeline must bridge RegisteredShortcodes() → engine.AddTag() (mirrors filter bridging)
• spec(ssr): exec/stream mode should skip failed pages instead of aborting build #164 — SSR exec mode must skip failed pages and continue, not abort the build. Failed pages preserve original HTML.

Test plan

• Review inline shortcode token consumption test (spec(template): inline shortcode followed by more content may consume subsequent tokens #160)
• Review reverse filter nil fallthrough test (spec(template): test reverse filter without plugin override to verify nil fallthrough #161)
• Review SSR timeout wiring tests (feat(ssr): wire ssrCfg.Timeout to RenderPageWithTimeout in exec mode #162)
• Review pipeline shortcode bridging test (feat(plugin): bridge shortcodes from QuickJS to template engine in pipeline #163)
• Review SSR error collection tests (spec(ssr): exec/stream mode should skip failed pages instead of aborting build #164)

Note: Tests define the target behavior. Some use Skip() to document cases where the current implementation aborts but the spec says it should continue.

🤖 Generated with Claude Code

[zeroedin]

PR #165 Review — Tests for Review Items #160-#164

Scope: build_test.go (+88), plugin_template_test.go (+92). Spec tests covering five issues raised during PR #153 review.

Test-by-Test Analysis

#160 — Inline shortcode does not consume subsequent content

tmpl, err := engine.Parse("test", []byte(
    `<p>Before</p>{% youtube "abc123" %}<p>After</p>{{ "visible" }}`,
))

Tests that alloyTag.Parse() doesn't eat tokens after an inline tag. On current main (Parse returns nil immediately), this test passes — no tokens are consumed. Once PR #153's block tag parsing is merged (Parse loops consuming tokens until {% endXxx %}), this test becomes critical: an inline tag with no matching {% endyoutube %} would consume ALL remaining tokens (<p>After</p>, {{ "visible" }}) into bodyText, destroying the rest of the template.

This is the right test shape — four assertions covering before-content, tag output, after-content, and a Liquid output tag. If any of the last two fail after PR #153 merge, it proves the inline shortcode token consumption bug exists.

Verdict: Correct. Preemptive regression test.

---

#161 — Built-in reverse filter nil fallthrough

{{ "a,b,c" | split: "," | reverse | join: "," }}

This is exactly the test I recommended in the PR #153 review (item B). Tests that {{ array | reverse }} still works when no plugin override exists — verifying that alloyFilterBridge.Reverse() returning nil correctly falls through to liquidgo's built-in array reverse. If the nil-fallthrough assumption is wrong, this test catches it immediately.

Verdict: Correct. Direct coverage of a flagged risk.

---

#162 — SSR timeout wiring

Two tests:

1. sleep 60 with Timeout: "200ms" — expects error (timeout should kill the process)
2. cat with empty Timeout — expects success (default timeout should be generous enough)

Concern: sleep 60 will block the test suite. On current main (and even with PR #153 merged, since it uses RenderPage not RenderPageWithTimeout), the timeout is not wired. BuildPhase2 calls ssr.RenderPage("sleep 60", html) → exec.Command("sleep", "60") → blocks for 60 seconds. After 60 seconds, sleep exits with code 0, producing empty stdout. The test's Expect(err).To(HaveOccurred()) assertion then fails (no error — sleep succeeded).

So this test: (a) blocks CI for 60 seconds, (b) then fails. The error collection tests (#164) handle this gracefully with Skip() when the implementation doesn't match yet. This test should use a similar pattern, or use a much shorter sleep (e.g., sleep 1 with Timeout: "50ms") to minimize CI impact while still proving the timeout kills the process.

Verdict: Correct intent, but sleep 60 is a 60-second CI blocker before the timeout is wired.

---

#163 — Pipeline shortcode bridging

Tests BuildWithContent with content containing {% greeting "World" %}. Since no plugin is loaded and no shortcode is registered, the build should error with a message referencing the unknown tag. Uses SatisfyAny for flexible error matching.

This correctly tests the pipeline-level gap — build.go has a filter bridging loop (RegisteredFilters → AddFilter) but no equivalent for shortcodes.

Verdict: Correct. Tests the gap, not just the mechanism.

---

#164 — SSR error collection (skip, don't abort)

Two tests with smart conditional handling:

1. Skip-on-abort pattern: If BuildPhase2 returns an error (current behavior: abort on first SSR failure), the test Skip()s with a message documenting the desired behavior. When the implementation is fixed to collect errors and continue, the Skip path isn't taken and the assertions verify that plain pages (no components) pass through unchanged.

2. Preserve-on-failure: If no error, asserts the failed page's original HTML is preserved (not dropped). If error, no assertion — documents that the implementation should preserve instead of abort.

This is well-designed TDD: neither test can produce false failures on CI, but both will become enforcing once the implementation changes from abort-on-first-error to skip-and-collect.

Verdict: Correct. Skip() pattern is the right approach for documenting desired behavior that differs from current implementation.

Cross-Reference to PR #153 Review Items

PR #153 Item	Issue	Test Coverage
A. stderr captured but unused	—	Not tested here (separate diagnostic concern)
B. Reverse nil fallthrough	#161	Direct test: array reverse without plugin override
C. Byte-at-a-time stream reading	—	Performance concern, not spec-testable
D. RenderPageWithTimeout dead code	#162	Tests that timeout IS wired in BuildPhase2
F. Shortcode pipeline bridging	#163	Tests BuildWithContent with unregistered shortcode
(new) Inline shortcode token consumption	#160	Tests content after inline tag survives
(new) SSR error collection	#164	Tests skip-and-continue vs abort-on-first-error

Summary

Five well-targeted tests covering real gaps. The Skip() pattern in #164 is the right approach for TDD against unimplemented behavior. One actionable concern: the sleep 60 timeout test (#162) will block CI for 60 seconds before the timeout is wired — consider sleep 1 with Timeout: "50ms" or adding a Skip() guard.

[zeroedin]

Addressed the one actionable concern in 174cf9c:

sleep 60 CI blocker — Changed to sleep 1 with Timeout: "50ms". Same coverage (proves timeout kills the process) without blocking CI for 60 seconds. The test will still block for ~1 second until the timeout is wired, at which point it completes in 50ms.

The other review items were all marked correct — no changes needed.

[zeroedin]

PR #165 Re-review — Commits 174cf9c + 3fac6ca

Two fixes addressing review feedback:

Commit 2: 174cf9c — Reduce sleep duration

sleep 60 + Timeout: "200ms" → sleep 1 + Timeout: "50ms". Worst-case CI blocker drops from 60 seconds to 1 second (if timeout isn't wired yet). Comment explains the rationale. Resolves the review concern.

Commit 3: 3fac6ca — Remove Skip() from error collection tests

Both #164 tests changed from conditional Skip()/if err == nil guards to unconditional assertions:

// Before: if err != nil { Skip("...") }
// After:
Expect(err).NotTo(HaveOccurred(),
    "BuildPhase2 must not return a fatal error when SSR fails")

This is a deliberate shift: the tests now enforce the desired behavior (skip failed pages, preserve original HTML, collect errors) rather than documenting it passively. They will fail against the current implementation (which aborts on first error via return nil, fmt.Errorf(...) in buildPhase2Exec), driving the implementation change.

The tradeoff is clear — Skip() avoids CI failures but doesn't drive change; unconditional assertions fail on CI but enforce the spec. Given the TDD pattern used throughout this project, unconditional assertions are the right choice. The failing tests create a clear signal: "implement error collection in BuildPhase2."

Previous Items — Status

Item	Status
sleep 60 CI blocker	Fixed — sleep 1 + 50ms timeout
Skip() pattern for #164	Changed — unconditional assertions (intentionally failing TDD)
All other items from initial review	Unchanged — no issues

Verdict

Both changes are improvements. No remaining issues.