Skip to content

fix: TestContainerSupport agent job detection matches manifest comments#25778

Merged
pelikhan merged 1 commit intomainfrom
copilot/fix-tests-again
Apr 11, 2026
Merged

fix: TestContainerSupport agent job detection matches manifest comments#25778
pelikhan merged 1 commit intomainfrom
copilot/fix-tests-again

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 11, 2026

TestContainerSupport subtests fail because strings.Contains(line, "agent:") matches the # gh-aw-manifest: JSON comment (which contains "agent:") before reaching the actual YAML job key. The test then exits the search prematurely on issues: thinking it's a new job.

  • Changed agent job detection from substring match to exact line match:
// Before: matches "agent:" anywhere in any line (including manifest JSON)
if strings.Contains(line, string(constants.AgentJobName)+":") {

// After: matches only the YAML job definition line
agentJobLine := "  " + string(constants.AgentJobName) + ":"
if line == agentJobLine {
  • Applied same fix to both TestContainerSupport and TestServicesSupport search loops

…ion to use exact line matching

The tests used strings.Contains(line, "agent:") which matched manifest JSON
comments containing "agent:" before reaching the actual YAML job definition.
This caused the test to exit the job search prematurely. Changed to exact
line matching with line == "  agent:" for precise YAML key detection.

Agent-Logs-Url: https://github.com/github/gh-aw/sessions/bd9c3d91-0b94-4305-bb05-5fc0569181c7

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
@pelikhan pelikhan marked this pull request as ready for review April 11, 2026 10:13
Copilot AI review requested due to automatic review settings April 11, 2026 10:13
@pelikhan pelikhan merged commit d8b09d3 into main Apr 11, 2026
54 of 55 checks passed
@pelikhan pelikhan deleted the copilot/fix-tests-again branch April 11, 2026 10:13
Copy link
Copy Markdown
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

Fixes flaky agent-job detection in integration tests by ensuring the scan for the agent job matches the YAML job definition line exactly (avoiding accidental matches inside the # gh-aw-manifest: JSON header comment).

Changes:

  • Replace strings.Contains(..., "agent:") with an exact match against the expected job key line (" agent:") in TestContainerSupport.
  • Apply the same exact-line job detection in TestServicesSupport.
Show a summary per file
File Description
pkg/workflow/container_services_test.go Tightens agent job start detection to avoid false-positives from gh-aw-manifest header comments when validating container/services sections.

Copilot's findings

Tip

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

  • Files reviewed: 1/1 changed files
  • Comments generated: 1

Comment on lines +144 to +150
agentJobLine := " " + string(constants.AgentJobName) + ":"
for i, line := range lines {
if strings.Contains(line, string(constants.AgentJobName)+":") {
if line == agentJobLine {
inMainJob = true
continue
}
if inMainJob && strings.HasPrefix(line, " ") && !strings.HasPrefix(line, " ") && line != " "+string(constants.AgentJobName)+":" {
if inMainJob && strings.HasPrefix(line, " ") && !strings.HasPrefix(line, " ") && line != agentJobLine {
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

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

The agent-job detection change is applied here, but the same substring-based job detection pattern still exists in pkg/workflow/environment_test.go (see around environment_test.go:140-146). Because the generated header can include container image refs like "/agent:" inside the single-line "# gh-aw-manifest: ..." comment, that test loop is still vulnerable to the same false-positive and may keep failing CI. Consider applying the same exact-line match (or a shared helper) across all tests that scan for the agent job start.

Copilot uses AI. Check for mistakes.
@github-actions github-actions Bot mentioned this pull request Apr 11, 2026
@github-actions
Copy link
Copy Markdown
Contributor

🧪 Test Quality Sentinel Report

Test Quality Score: 100/100

Excellent test quality

Metric Value
New/modified tests analyzed 2
✅ Design tests (behavioral contracts) 2 (100%)
⚠️ Implementation tests (low value) 0 (0%)
Tests with error/edge cases 2 (100%)
Duplicate test clusters 0
Test inflation detected No
🚨 Coding-guideline violations None

Test Classification Details

Test File Classification Issues Detected
TestContainerSupport pkg/workflow/container_services_test.go ✅ Design None — behavioral contract verified
TestServicesSupport pkg/workflow/container_services_test.go ✅ Design None — behavioral contract verified

Summary of Changes

This PR fixes the agent-job detection logic inside both test functions. Previously, the loop used strings.Contains(line, agentJobName+":") which could falsely match YAML comments or other lines that contained the agent job name string. The fix extracts a precise line pattern (agentJobLine := " " + string(constants.AgentJobName) + ":") and uses strict equality (line == agentJobLine) to locate the job boundary.

Both TestContainerSupport and TestServicesSupport are table-driven integration tests that:

  1. Write a real workflow markdown file to a temp directory
  2. Invoke compiler.ParseWorkflowFile() and compiler.generateYAML() — real production code, no mocks
  3. Assert on the observable output (the generated YAML) — that container: / services: sections appear correctly indented inside the agent job
  4. Include an explicit "no container/no services" edge-case row that asserts the section is absent from the YAML

These are clean behavioral-contract tests. The fix makes the test more correct without weakening the contract being verified.


Language Support

Tests analyzed:

  • 🐹 Go (*_test.go): 2 tests (integration — //go:build integration) ✅ build tag present

Verdict

Check passed. 0% of new/modified tests are implementation tests (threshold: 30%). Both tests verify observable compiler output, cover the happy path and the absence-of-feature edge case, and use real component interactions with no mock libraries.


📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

  • Assert on observable outputs, return values, or state changes
  • Cover error paths and boundary conditions
  • Would catch a behavioral regression if deleted
  • Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

  • Assert on internal function calls (mocking internals)
  • Only test the happy path with typical inputs
  • Break during legitimate refactoring even when behavior is correct
  • Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

🧪 Test quality analysis by Test Quality Sentinel · ● 645.3K ·

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

✅ Test Quality Sentinel: 100/100. Test quality is excellent — 0% of new/modified tests are implementation tests (threshold: 30%). Both modified tests are behavioral-contract integration tests that exercise real compiler code, cover the happy path and an edge case (absent feature), and now use precise job-line detection to eliminate false-positive matches on manifest comments.

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.

3 participants