Skip to content

Resolve FIXME-skipped tests in compiler_expression_size_test.go#19311

Merged
pelikhan merged 2 commits intomainfrom
copilot/resolve-fixme-skipped-tests
Mar 3, 2026
Merged

Resolve FIXME-skipped tests in compiler_expression_size_test.go#19311
pelikhan merged 2 commits intomainfrom
copilot/resolve-fixme-skipped-tests

Conversation

Copy link
Contributor

Copilot AI commented Mar 3, 2026

Two integration tests in compiler_expression_size_test.go were permanently skipped because the tests incorrectly assumed large markdown body content would be embedded in the generated YAML. In normal compilation mode, the markdown body becomes a short {{#runtime-import ...}} macro line — never triggering validateExpressionSizes().

Changes

  • inlined-imports: true in test frontmatter: Forces the markdown body to be embedded directly in the generated YAML at compile time. A single 25,000-char line in the markdown body then appears as a ~25KB YAML line, exceeding MaxExpressionSize (21,000 bytes) and correctly triggering the validation.
  • Remove t.Skip("FIXME: ...") from both tests — they now exercise the actual validation path.
  • Remove compiler.SetSkipValidation(false): Expression size validation runs unconditionally (before schema validation), so this call was misleading and irrelevant.
  • Remove unused safe-outputs section from both test workflows.
---
on: push
inlined-imports: true   # forces markdown body into YAML → triggers line-length check
tools:
  github:
    allowed: [list_issues]
---

# Large Content Test Workflow

xxxx...25000 chars on one line...  # becomes a ~25KB YAML line → validateExpressionSizes() fires
Original prompt

This section details on the original issue you should resolve

<issue_title>[plan] Resolve FIXME-skipped tests in compiler_expression_size_test.go</issue_title>
<issue_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

  1. Read pkg/workflow/compiler_expression_size_test.go to understand what the skipped tests were trying to validate
  2. Locate validateExpressionSizes() (likely in pkg/workflow/) and understand what it actually validates today
  3. 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
  4. 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 large run: blocks that exceed the expression size limit). Replace the t.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.

Validation

go build ./...
go vet ./...
make fmt
make test-unit

Acceptance 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-unit passes with no regressions

Generated by Plan Command for issue #discussion #19223

  • expires on Mar 4, 2026, 2:57 PM UTC

Comments on the Issue (you are @copilot in this section)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Use inlined-imports: true frontmatter to force markdown body embedding
in generated YAML, allowing validateExpressionSizes() to be triggered
by large single-line content that exceeds the 21KB limit.

- Remove t.Skip("FIXME: ...") from both previously-skipped tests
- Add inlined-imports: true to test workflow frontmatter
- Remove misleading SetSkipValidation(false) call (expression size
  validation runs unconditionally regardless of skipValidation)
- Remove unused safe-outputs section from both test workflows
- Update test comments to explain the inlined-imports mechanism"

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Investigate and resolve FIXME-skipped tests in compiler_expression_size_test.go Resolve FIXME-skipped tests in compiler_expression_size_test.go Mar 3, 2026
@pelikhan pelikhan marked this pull request as ready for review March 3, 2026 02:34
Copilot AI review requested due to automatic review settings March 3, 2026 02:34
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR resolves two permanently-skipped FIXME integration tests in compiler_expression_size_test.go. The tests were previously skipped because they incorrectly assumed markdown body content would be embedded in the generated YAML (it normally becomes a {{#runtime-import ...}} macro). The fix adds inlined-imports: true to the test workflow frontmatter, which forces the markdown body to be embedded directly in the YAML at compile time — causing the 25,000-character single-line content to exceed MaxExpressionSize (21,000 bytes) and correctly trigger validateExpressionSizes().

Changes:

  • Added inlined-imports: true to both test workflows to force inline embedding of the markdown body content, making the 25KB single-line content exceed MaxExpressionSize and trigger the validation error.
  • Removed t.Skip("FIXME: ...") from both skipped tests so they now actively exercise the expression size validation path.
  • Removed compiler.SetSkipValidation(false) (it was misleading — validateExpressionSizes() runs unconditionally regardless of skipValidation) and removed the unused safe-outputs section from both test workflows.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@pelikhan pelikhan merged commit 0ad63a0 into main Mar 3, 2026
110 checks passed
@pelikhan pelikhan deleted the copilot/resolve-fixme-skipped-tests branch March 3, 2026 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[plan] Resolve FIXME-skipped tests in compiler_expression_size_test.go

3 participants