spec(test): add failing Liquid rendering tests for built-in filters#204
Conversation
) Filter unit tests pass but filters may fail when called through the Liquid template engine. Add tests that render templates using findRE, replaceRE, contains, and newline_to_br through the Liquid engine. Two tests fail red on current main: - replaceRE: returns the replacement string instead of performing substitution - newline_to_br: produces empty output instead of <br> tags Two tests pass (control cases): - findRE: correctly returns matches - contains: correctly returns truthy value for conditionals Tests are in internal/template/ which compiles and runs independently (the integration test suite has unrelated compilation issues). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zeroedin
left a comment
There was a problem hiding this comment.
PR #204 Review — Liquid Engine Rendering Tests for Built-In Filters (#199)
Scope: liquid_test.go (+49). Four end-to-end filter tests through the Liquid rendering pipeline.
Why These Tests
Filters can pass unit tests (correct Go function logic) but fail through the Liquid engine due to:
- Registration mismatch (
knownLiquidFiltersvsdynamicFilters) - Type coercion between liquidgo's internal types and the filter function's expected types
- Filter dispatch routing (bridge method vs
PluginFilterrewrite vs liquidgo native)
These tests exercise the full path: engine.Parse → template with filter → tpl.Render → filter dispatch → rendered output.
Tests
1. findRE — Regex match extraction
{{ "hello world 123" | findRE: "[0-9]+" }} → contains "123". Tests that findRE receives the string input and regex pattern, executes the match, and returns results through the Liquid engine. findRE is an alloy custom filter (not a liquidgo built-in), so it goes through the alloyFilterBridge dispatch.
2. replaceRE — Regex substitution
{{ "hello world" | replaceRE: "world", "alloy" }} → contains "hello alloy". Two-arg filter (pattern + replacement). The second assertion (NotTo(Equal("world"))) guards against the failure mode where the filter returns just the replacement argument instead of the transformed input — this would indicate the args were shifted.
3. contains — Boolean in conditional
{% if "hello world" | contains: "world" %}FOUND{% endif %} → contains "FOUND". Tests that contains returns a value that Liquid treats as truthy in a conditional. This is a subtle integration point — the filter must return a Go bool or a value that liquidgo's truthiness check accepts.
4. newline_to_br — Built-in liquidgo filter
{{ "hello\nworld" | newline_to_br }} → contains "<br". Tests a liquidgo StandardFilter through the engine. The literal \n in the template string is interesting — whether it's interpreted as a newline depends on how liquidgo handles escape sequences in string literals. If liquidgo treats \n literally (two characters), the filter won't find a newline to convert.
Observation
The newline_to_br test uses "hello\nworld" as a Liquid string literal. In Liquid, string literals don't process escape sequences — \n is literally backslash-n, not a newline. The test might need to use a template variable with a real newline instead:
result, err := tpl.Render(map[string]interface{}{
"text": "hello\nworld", // Go string with actual newline
})With {{ text | newline_to_br }} in the template. If the test passes as-is, it means liquidgo does interpret \n in string literals, which would be non-standard Liquid behavior. If it fails, it's because the input has no actual newlines. Worth verifying.
Verdict
Good integration tests that catch the registration→dispatch→rendering pipeline failures that unit tests miss. The replaceRE arg-shift guard and contains truthiness test are particularly valuable. One potential issue with the newline_to_br test's escape sequence handling — may need a template variable instead of a string literal. No blocking issues.
There was a problem hiding this comment.
Pull request overview
Adds Liquid-engine-level regression tests for Issue #199 to catch mismatches between built-in filter unit behavior and actual Liquid template rendering/dispatch.
Changes:
- Introduces Liquid rendering tests for
findRE,replaceRE,contains, andnewline_to_br. - Verifies filter behavior via
tmpl.NewLiquidEngine().Parse(...).Render(...)rather than direct filter function calls.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Adds Liquid-rendering (engine-level) tests to exercise built-in filters through the actual Liquid dispatch path, aiming to reproduce/lock in regressions described in Issue #199 that aren’t caught by existing unit-level filter tests.
Changes:
- Add a new Ginkgo
Contextcovering built-in filter behavior when invoked via Liquid template rendering. - Add four tests for
findRE,replaceRE,contains, andnewline_to_br.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address copilot review: - contains: add negative case (substring absent) to prove the filter returns a boolean, not the input string. Both cases being truthy would mask the bug. Now fails red (YES|NO expected, gets different). - newline_to_br: pass string via render context instead of template literal — backtick \n is literal backslash-n, not a real newline. With real newline the filter works correctly. Result: replaceRE and contains fail red, findRE and newline_to_br pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…204) Address remaining copilot review: - Call RegisterBuiltinFilters(engine) in all 4 filter tests — without it, the filter bridge has nothing to dispatch to - Strengthen findRE: assert output is NOT the original input (proves filter ran, not passthrough). "123" is in the input string, so ContainSubstring alone was too weak. Now 3 tests fail red: findRE, replaceRE, contains. newline_to_br passes (works correctly with real newlines via context). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Spec work for #199, with notes on #200, #201, #141, #113, #107
The developer kicked back 6 issues because "all tests pass." The existing tests are either unit-level (filter functions called directly) or in the integration test suite which has unrelated compilation issues.
This PR: #199 — Built-in filters through Liquid rendering
Added 4 tests in
internal/template/liquid_test.gothat render templates through the Liquid engine:findREreturns matchesreplaceREperforms substitutioncontainsreturns booleannewline_to_brconverts newlinesTests are in
internal/template/which compiles and runs independently.Remaining issues (#200, #201, #141, #113, #107)
These need pipeline-level tests (
pipeline.BuildorBuildWithContent). The integration test suite (test/integration/) has existing tests for all of them but the suite has an unrelated compilation error (undefined: pipelineincrosscutting_test.go). Once that compilation issue is fixed, the existing tests should be runnable. The developer should fix the compilation issue first — the failing tests already exist.Test plan
go test ./internal/template/... -v— 2 tests fail red (replaceRE, newline_to_br)🤖 Generated with Claude Code