Skip to content

test(EC-1824): add acceptance tests for multi-arch volatile exceptions#3333

Open
robnester-rh wants to merge 2 commits into
conforma:mainfrom
robnester-rh:EC-1824
Open

test(EC-1824): add acceptance tests for multi-arch volatile exceptions#3333
robnester-rh wants to merge 2 commits into
conforma:mainfrom
robnester-rh:EC-1824

Conversation

@robnester-rh

@robnester-rh robnester-rh commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Add acceptance tests for multi-arch volatileConfig componentNames excludes
🧪 Tests 🕐 20-40 Minutes

Grey Divider

Walkthroughs

User Description

Summary

  • Adds two acceptance test scenarios verifying volatile config componentNames excludes work with multi-arch expanded component names
  • Tests the originalComponentName() wiring in Criteria.get() end-to-end through validate image
  • Uses --images with ApplicationSnapshot file to set component names to the expanded format (e.g., foo-sha256:<digest>-arm64)

Test plan

  • Positive: exclude matches when expanded name's base matches componentNames (exit 0)
  • Negative: exclude does NOT match when base name differs (exit 1)
  • Both acceptance tests pass locally

resolves: EC-1824

🤖 Generated with Claude Code

AI Description
• Add end-to-end acceptance coverage for multi-arch expanded component name excludes.
• Verify Criteria.get() uses originalComponentName() so volatileConfig componentNames still match.
• Exercise validate image via --images ApplicationSnapshot inputs and assert exit codes + snapshots.
Diagram
graph TD
  A["Acceptance scenarios"] --> B["ec validate image"] --> C["Load --images snapshot"] --> D["Expanded component name"] --> E["Criteria.get(): originalComponentName"] --> F{"Exclude matches componentNames?"}
  F -->|"yes"| G["Skip excluded checks"] --> H["Exit status"]
  F -->|"no"| I["Run failing rule"] --> H
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Add focused unit tests for component name normalization
  • ➕ Faster, less brittle than acceptance tests
  • ➕ Pinpoints behavior of originalComponentName() and matching logic directly
  • ➖ May miss CLI wiring/regressions in validate image path
  • ➖ Does not validate ApplicationSnapshot/--images integration
2. Add a smaller integration test around Criteria.get() + volatile config parsing
  • ➕ Covers wiring without full signature/attestation setup
  • ➕ Less runtime and fewer external test fixtures
  • ➖ Still may not cover the exact validate image execution path and output formatting
  • ➖ Requires bespoke harness if one doesn’t already exist

Recommendation: Keep the acceptance tests as implemented because they validate the bug-prone end-to-end path (validate image + snapshot-provided expanded names + volatile excludes). If future failures become flaky/slow, consider complementing with a narrow unit test for name normalization to reduce reliance on full acceptance setup.

Grey Divider

File Changes

Tests (1)
validate_image.feature Add EC-1824 acceptance scenarios for multi-arch volatile excludes +106/-0

Add EC-1824 acceptance scenarios for multi-arch volatile excludes

• Adds two new acceptance test scenarios to ensure volatileConfig 'exclude.componentNames' matches against the original (base) component name when validate image operates on expanded multi-arch component names. Uses '--images' with ApplicationSnapshot JSON to inject expanded component names and asserts correct exit statuses and output snapshots for both matching and non-matching cases.

features/validate_image.feature


Grey Divider

Qodo Logo

Add two acceptance test scenarios verifying that volatile config
componentNames excludes work correctly with multi-arch expanded
component names (e.g., "foo-sha256:<digest>-arm64").

The tests use --images with an ApplicationSnapshot file to set
component names to the expanded format, validating the
originalComponentName() wiring in Criteria.get() end-to-end.

Scenario A: exclude matches when expanded name's base matches componentNames
Scenario B: exclude does NOT match when base name differs

resolves: EC-1824

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 7efd1af1-cc2d-4d2b-b021-46d53526ac54

📥 Commits

Reviewing files that changed from the base of the PR and between 7bf6430 and 9817ee4.

📒 Files selected for processing (1)
  • features/validate_image.feature
🚧 Files skipped from review as they are similar to previous changes (1)
  • features/validate_image.feature

📝 Walkthrough

Walkthrough

Adds an EC-1824 comment and two Gherkin scenarios that test whether volatileConfig.exclude entries keyed by original component names match expanded multi-arch component names in ec validate image --images runs; one scenario expects success, the other expects failure.

Changes

Multi-arch volatile config matching

Layer / File(s) Summary
Multi-arch volatile config exclude matching tests
features/validate_image.feature
Adds comment documenting EC-1824 multi-arch name matching, then introduces two ec validate image scenarios: the first verifies volatileConfig.exclude rules keyed by original component name multi-arch-test match the expanded multi-arch format and succeed; the second verifies exclude rules do not match a different component and fail.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: adding acceptance tests for multi-arch volatile exceptions related to EC-1824.
Description check ✅ Passed The description is directly related to the changeset, providing detailed context about the test scenarios, objectives, and implementation approach for multi-arch volatile config excludes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@qodo-for-conforma

Copy link
Copy Markdown

Code Review by Qodo

Grey Divider

Sorry, something went wrong

We weren't able to complete the code review on our side. Please try again

Grey Divider

Qodo Logo

@robnester-rh robnester-rh requested a review from cuipinghuo June 8, 2026 13:09
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 8, 2026

Copy link
Copy Markdown

Review

Findings

Info

  • [test-inadequate] features/validate_image.feature:1118 — The two scenarios provide good positive/negative coverage for multi-arch expanded component name matching. A potential additional edge case would test a component name containing a sha256-like substring that is NOT a multi-arch expanded name (e.g., without the sha256: prefix pattern), to verify originalComponentName() correctly identifies the expansion boundary rather than doing a simple prefix match.

  • [sub-agent-failure] — The style-conventions sub-agent did not return findings: model claude-sonnet-4-5@20250929 unavailable on Vertex deployment. This test-only PR's style was verified by the correctness sub-agent to follow established codebase patterns.

  • [sub-agent-failure] — The intent-coherence sub-agent did not return findings: model claude-sonnet-4-5@20250929 unavailable on Vertex deployment. The PR references EC-1824 (Jira) in the title, is test-only with no production code changes, and the scope is consistent with the claimed tier.

Previous run

Review

Findings

Low

  • [commit-message-type] N/A — PR title uses feat() prefix but contains only test additions (no production code). Recent repo commits use chore() for similar non-production changes. Consider using test(EC-1824): for consistency with conventional commit semantics.

Info

  • [test-adequacy] features/validate_image.feature:1118 — The tests validate originalComponentName() wiring by directly supplying component names in the expanded multi-arch format (foo-sha256:<digest>-<arch>). This tests volatile config exclude matching in Criteria.get() without exercising the full imageIndexWorker expansion path. Unit tests in criteria_test.go cover originalComponentName() in isolation, and together the coverage is reasonable.
  • [test-integrity] features/validate_image.feature — No existing tests are modified, weakened, or removed. The diff is purely additive, appending two new scenarios and their corresponding snapshot data.

Comment thread features/validate_image.feature Outdated
Then the exit status should be 0
Then the output should match the snapshot

# Tests for EC-1824: multi-arch per-component volatile exceptions.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[info] test-adequacy

The tests validate originalComponentName() wiring by directly supplying component names in the expanded multi-arch format. This tests volatile config exclude matching in Criteria.get() without exercising the full imageIndexWorker expansion path. Unit tests in criteria_test.go cover originalComponentName() in isolation, and together the coverage is reasonable.

@cuipinghuo cuipinghuo Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitchpick: to keep the comments neat maybe we don't need such detailed comments.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point — trimmed it down to two lines in 9817ee4.

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Jun 8, 2026
@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
acceptance 53.63% <ø> (+0.08%) ⬆️
generative 16.94% <ø> (ø)
integration 27.91% <ø> (ø)
unit 69.02% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

reference: EC-1824

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@robnester-rh robnester-rh changed the title feat(EC-1824): add acceptance tests for multi-arch volatile exceptions test(EC-1824): add acceptance tests for multi-arch volatile exceptions Jun 8, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 8, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 5:28 PM UTC · Completed 5:34 PM UTC
Commit: 47d3320 · View workflow run →

Then the exit status should be 0
Then the output should match the snapshot

# EC-1824: verify volatile config componentNames excludes work with

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[info] test-inadequate

The two scenarios provide good positive/negative coverage for multi-arch expanded component name matching. A potential additional edge case would test a component name containing a sha256-like substring that is NOT a multi-arch expanded name (e.g., without the sha256: prefix pattern), to verify originalComponentName() correctly identifies the expansion boundary rather than doing a simple prefix match.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels Jun 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge All reviewers approved — ready to merge size: XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants