-
Notifications
You must be signed in to change notification settings - Fork 296
Closed
Labels
Description
Objective
Investigate and resolve two // FIXME-skipped tests in pkg/workflow/compiler_expression_size_test.go. These tests document a known design gap and need to be either redesigned with valid test scenarios or removed with an explanatory comment.
Context
Discussion #19223 identified two test functions in pkg/workflow/compiler_expression_size_test.go (around lines 63 and 113) that are skipped with this comment:
t.Skip("FIXME: Markdown content is not embedded in generated YAML, so this validation doesn't apply.
The validateExpressionSizes() function checks individual YAML lines, but markdown content
from .md files is not stored in the .lock.yml as environment variables or expressions.
This test needs to be redesigned or removed.")
These are different from the require()-migration zombie tests — they document a real design question about validateExpressionSizes().
Investigation Steps
- Read
pkg/workflow/compiler_expression_size_test.goto understand what the skipped tests were trying to validate - Locate
validateExpressionSizes()(likely inpkg/workflow/) and understand what it actually validates today - Determine: can
validateExpressionSizes()be triggered by real workflow content? Check:- Very long
env:variable values in frontmatter - Large
run:step scripts - Long expression strings in step conditions or outputs
- Very long
- Based on findings, choose one of:
- Option A: Redesign the tests to use workflow content that actually produces large YAML expressions (e.g., long
env:values or largerun:blocks that exceed the expression size limit). Replace thet.Skip()with a real test. - Option B: Delete the test functions and add a comment in the file explaining that
validateExpressionSizes()cannot be triggered by markdown-sourced content, so the validation is effectively unreachable in practice.
- Option A: Redesign the tests to use workflow content that actually produces large YAML expressions (e.g., long
Validation
go build ./...
go vet ./...
make fmt
make test-unitAcceptance Criteria
- Both FIXME-skipped test functions either replaced with real tests or removed with explanatory comment
- If redesigned: new tests actually exercise
validateExpressionSizes()with triggering input - If removed: comment explains the limitation of the validation function
-
make test-unitpasses with no regressions
Generated by Plan Command for issue #discussion #19223
- expires on Mar 4, 2026, 2:57 PM UTC
Reactions are currently unavailable