Skip to content

fix: update TestMCPGatewayVersionFromFrontmatter to resolve pinned container image#28144

Merged
pelikhan merged 1 commit intomainfrom
copilot/fix-github-actions-workflow-infra
Apr 23, 2026
Merged

fix: update TestMCPGatewayVersionFromFrontmatter to resolve pinned container image#28144
pelikhan merged 1 commit intomainfrom
copilot/fix-github-actions-workflow-infra

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 23, 2026

Summary

Fixes the failing Integration: Workflow Infra CI job (check run 72739305683).

Root Cause

TestMCPGatewayVersionFromFrontmatter subtests no_version_specified_-_should_use_default and empty_version_string_-_should_use_default were failing because:

  1. The test built expectedImage = "ghcr.io/github/gh-aw-mcpg:v0.2.30" (bare tag)
  2. collectDockerImages applies embedded container pins from pkg/actionpins/data/action_pins.json
  3. action_pins.json has a pin for v0.2.30: ghcr.io/github/gh-aw-mcpg:v0.2.30@sha256:e950e6d39f003862d33bfb8d4eb93e242d919cf6ca874b90728e5e0ea7434c6f
  4. The actual returned image included the digest, so assert.Equal failed

Fix

Resolve the expected image through lookupContainerPin before asserting equality, so the test accounts for digest pinning while still validating that the correct version tag is used.

All 5 TestMCPGatewayVersionFromFrontmatter subtests now pass.

…ntainer image

The test was comparing bare container:tag against the output of collectDockerImages,
which applies embedded container pins from action_pins.json. For v0.2.30 (the default),
the embedded pin returns container:tag@sha256:... causing the equality assertion to fail.

Fix: resolve the expected image through lookupContainerPin before asserting, so the test
accounts for digest pinning while still validating the correct version is used.

Agent-Logs-Url: https://github.com/github/gh-aw/sessions/5ab849a0-0bfd-4f29-b06b-02949f96e6a4

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI requested a review from pelikhan April 23, 2026 18:31
@pelikhan pelikhan marked this pull request as ready for review April 23, 2026 18:32
Copilot AI review requested due to automatic review settings April 23, 2026 18:32
@pelikhan pelikhan merged commit 4efcb3f into main Apr 23, 2026
@pelikhan pelikhan deleted the copilot/fix-github-actions-workflow-infra branch April 23, 2026 18:32
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

Updates an integration test to account for container digest pinning so CI no longer fails when collectDockerImages returns digest-pinned image references.

Changes:

  • Resolves the expected MCP gateway container image through lookupContainerPin before asserting equality against collectDockerImages output.
Show a summary per file
File Description
pkg/workflow/mcp_setup_generator_test.go Adjusts expected MCP gateway image assertions to handle embedded digest pinning.

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 205 to +210
expectedImage := constants.DefaultMCPGatewayContainer + ":" + tt.expectedVersion
// collectDockerImages applies embedded container pins when available, so resolve
// the pinned reference for the expected image if one exists.
if pin, ok := lookupContainerPin(expectedImage, nil); ok && pin.PinnedImage != "" {
expectedImage = pin.PinnedImage
}
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

expectedImage is mutated to the digest-pinned reference and then reused in Test 2 (assert.Contains(setupOutput, expectedImage, ...)). Since the rendered YAML always contains the pinned image in the download step, this assertion can pass even if buildMCPGatewayContainerCommand (the actual gateway docker run command) uses the wrong tag/version. To keep the original intent, keep two expectations: (1) expectedTaggedImage := <container>:<version> for validating the gateway command, and (2) expectedPinnedImage for validating collectDockerImages output (download step).

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

🧪 Test Quality Sentinel Report

Test Quality Score: 95/100

Excellent test quality

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

Test Classification Details

Test File Classification Issues Detected
TestMCPGatewayVersionFromFrontmatter pkg/workflow/mcp_setup_generator_test.go:117 ✅ Design None

Analysis

This PR modifies TestMCPGatewayVersionFromFrontmatter by adding 5 lines that resolve the pinned container image reference before asserting on the expected docker image.

What changed: The test previously compared container:version directly against collectDockerImages() output, but that function applies embedded container pins (digest pinning), causing the assertion to fail when a pin exists. The fix calls lookupContainerPin() to resolve the pinned reference before asserting.

Design invariant enforced: The test verifies that the MCP gateway container version specified in frontmatter (or the default) is faithfully used in both the docker predownload image list and the MCP gateway setup command output — a behavioral contract that users of the sandbox.mcp.version frontmatter field rely on.

Value if deleted: High — a regression where frontmatter version is ignored (or wrongly overridden) would go undetected.

Edge cases covered (table-driven, 5 rows):

  • Custom version (v0.0.5)
  • No version → default
  • Empty string → default
  • "latest" preserved
  • Alternative format (1.2.3)

The fix scores 95 rather than 100 only because the -5 reflects that this is a narrow fix rather than newly added behavioral coverage; the test itself is excellent.


Language Support

Tests analyzed:

  • 🐹 Go (*_test.go): 1 test — integration (//go:build integration) ✓

Verdict

Check passed. 0% of new/modified tests are implementation tests (threshold: 30%). Build tag is correct (//go:build integration). No assertion-message violations. No mock library usage.

📖 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

References: §24852111962

🧪 Test quality analysis by Test Quality Sentinel · ● 754.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: 95/100. Test quality is excellent — 0% of new/modified tests are implementation tests (threshold: 30%). The fix correctly updates TestMCPGatewayVersionFromFrontmatter to resolve pinned container image references before asserting, making the test accurate without sacrificing behavioral coverage.

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