doc: E2E Test Coverage Proposal#296
Conversation
📝 WalkthroughWalkthroughAdds a documentation-only E2E test coverage proposal recording the current baseline, a prioritized matrix of 61 test scenarios, CI/workflow execution and trigger changes, a phased implementation plan, and future expansion goals. ChangesE2E Test Coverage Proposal
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| | `catalogimage` | #12, #53–55, #61 | `/e2e-catalogimage` PR comment | Kind + OpenShift | ~3 min | | ||
| | `traditional` | #23 | `/e2e-traditional` PR comment | OpenShift only | ~30 min | | ||
| | `bootc` | Existing bootc lane | `/e2e-bootc` PR comment | Kind + OpenShift | ~20 min | | ||
| | `auth` | #50, #51 + existing auth tests | Nightly + `/e2e-auth` | OpenShift only | ~5 min | |
There was a problem hiding this comment.
this been tested on existing CRC OpenShift local cluster. at took even <5minutes
| |-------|-------|---------|---------|---------| | ||
| | `smoke` | #1–19 (superset of `operator`) | Every PR push (default) | Kind + OpenShift | < 2 min | | ||
| | `operator` | #1–5 (subset of `smoke`, existing `operator_test.go` tests only) | `/e2e-operator` PR comment | Kind + OpenShift | ~3 min | | ||
| | `catalogimage` | #12, #53–55, #61 | `/e2e-catalogimage` PR comment | Kind + OpenShift | ~3 min | |
There was a problem hiding this comment.
it's not an implemented feature, we can skip it
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/e2e/e2e-test-coverage-proposal.md (1)
150-150:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRuntime target for
smokeis likely under-estimated and conflicts with the proposed scope.
smokeis defined as tests#1–19, but runtime is listed as< 2 min; that seems inconsistent with both the expanded scope and the current benchmark baselines. Please re-baseline this estimate (or reduce smoke scope) to avoid setting incorrect CI expectations.Also applies to: 173-173
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/e2e-test-coverage-proposal.md` at line 150, The runtime estimate for the "smoke" target (tests `#1`–19) is inconsistent with its defined scope and current baselines: update the "smoke" entry so the CI expectation is realistic by either reducing the scope of `smoke` (e.g., move some tests out of tests `#1`–19 into a separate target) or re-baselining the runtime from "< 2 min" to an empirically measured value that matches the expanded scope; make the change where the "smoke" row is defined (the table cell containing `smoke` and tests `#1`–19 and the runtime value) and propagate the same correction to the other occurrence referenced (line ~173).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/e2e-test-coverage-proposal.md`:
- Line 47: Update the awkward wording on the unimplemented coverage line: change
the line that currently reads "No CatalogImage, ContainerBuild, Workspace,
ImageReseal, or Flash coverage (not implemented feature)" to a clearer phrasing
such as "No coverage for CatalogImage, ContainerBuild, Workspace, ImageReseal,
or Flash (features not yet implemented)"; locate the sentence referencing
CatalogImage, ContainerBuild, Workspace, ImageReseal, and Flash and replace the
parenthetical to the pluralized, tightened form.
---
Duplicate comments:
In `@test/e2e/e2e-test-coverage-proposal.md`:
- Line 150: The runtime estimate for the "smoke" target (tests `#1`–19) is
inconsistent with its defined scope and current baselines: update the "smoke"
entry so the CI expectation is realistic by either reducing the scope of `smoke`
(e.g., move some tests out of tests `#1`–19 into a separate target) or
re-baselining the runtime from "< 2 min" to an empirically measured value that
matches the expanded scope; make the change where the "smoke" row is defined
(the table cell containing `smoke` and tests `#1`–19 and the runtime value) and
propagate the same correction to the other occurrence referenced (line ~173).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 69fcf81d-5b0b-4cb3-a3c4-574afca517be
📒 Files selected for processing (1)
test/e2e/e2e-test-coverage-proposal.md
|
@ambient-code please review |
![ambient-code[bot]](https://avatars.githubusercontent.com/u/231559327?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a well-structured and thorough E2E testing proposal. The phased rollout plan, label strategy, and CI workflow changes are well-thought-out. The gap analysis is a valuable contribution. A few factual corrections needed before this is ready to merge:
Factual Issues
1. API route count is significantly undercounted (24 → 44+)
The proposal claims "24 total" Build API routes. Actual count from the source:
| Group | Routes | Source |
|---|---|---|
| Builds | 10 | server.go |
| Flash | 4 | server.go |
| Container Builds | 5 | server.go |
| Config | 1 | server.go |
| Auth | 1 | server.go |
| Infrastructure (healthz, openapi, metrics) | 3 | server.go |
| Sealed/ImageReseal | 4 | sealed.go |
| Workspace | 12 | workspace.go |
| Catalog | 6 | catalog/routes.go |
| Total | 46 |
This affects the coverage percentage calculations in the Coverage Gap Analysis and Summary tables.
2. Workspace API routes undercounted (9 → 12)
The Coverage Gap Analysis table says Workspace has 9 API routes. Actual count from workspace.go: POST (create), GET (list), GET/:name, DELETE/:name, POST/:name/start, POST/:name/stop, POST/:name/sync, POST/:name/sync/plan, POST/:name/exec, GET/:name/shell, POST/:name/deploy, PUT/:name/lease = 12 routes.
3. "CatalogImage not yet implemented" is incorrect
The "What Is NOT Tested Today" section says "CatalogImage not yet implemented", but it is — there is a full controller (internal/controller/catalogimage/) with audit, metrics, and publisher modules, plus 6 API routes in internal/buildapi/catalog/routes.go (list, create, get, delete, verify, publish). The CRD controller and API are functional; what's missing is E2E test coverage, not the implementation.
4. Flash and Catalog missing from Coverage Gap Analysis table
The per-component table omits Flash (4 API routes in server.go) and Catalog (6 API routes in catalog/routes.go) as separate line items. These should be listed to give a complete picture.
Observations (non-blocking)
- The phasing is sensible — smoke first, then core lifecycle, then extended coverage.
- Phase 1 smoke suite target of <2 min CI time on every PR is a meaningful improvement over the current ~30 min full suite.
- Test #12 (CatalogImage reaches Available) depends on public registry access — worth noting this may need a fallback or skip in air-gapped environments.
- The "Future Planning" section's 90% coverage target is ambitious given the current ~8% baseline. The intermediate 40% and 70% milestones are realistic checkpoints.
- Consider adding a note about test data/fixture management strategy (e.g., shared manifests, test image references) since many tests will need similar setup.
| | Build API | — | — | 24 total | 0 (healthz only) | ~4% | | ||
| | Authentication | — | — | 1 | 3 (OIDC only, OpenShift) | ~30% | | ||
| | Bootc Build | — | — | — | 1 (full build) | ~80% | | ||
| | **Total** | **7** | **7** | **24** | **8** | **~8%** | |
There was a problem hiding this comment.
The route count here is 24 but the actual total across all Build API route registrations is 46 (see server.go, sealed.go, workspace.go, and catalog/routes.go). This also affects the ~4% coverage estimate.
| | **Total** | **7** | **7** | **24** | **8** | **~8%** | | |
| | Build API | — | — | 46 total | 0 (healthz only) | ~2% | |
| | Workspace | 1 | 1 | 9 | 0 | 0% | | ||
| | ImageReseal | 1 | 1 | 4 | 0 | 0% | | ||
| | OperatorConfig | 1 | 1 (48+ handlers) | 1 | 4 (resource existence only) | ~10% | | ||
| | Build API | — | — | 24 total | 0 (healthz only) | ~4% | |
There was a problem hiding this comment.
Workspace has 12 API routes, not 9. Missing: POST /:name/sync/plan, POST /:name/exec, PUT /:name/lease.
| | Build API | — | — | 24 total | 0 (healthz only) | ~4% | | |
| | Workspace | 1 | 1 | 12 | 0 | 0% | |
| - No cleanup/garbage-collection tests (owner references, TTL expiry, finalizers) | ||
| - No package mode AIB disk image build test | ||
| - No smoke test label — every PR runs the full suite (~30 min) | ||
| - No CatalogImage, ContainerBuild, Workspace, ImageReseal, or Flash coverage (CatalogImage not yet implemented) |
There was a problem hiding this comment.
CatalogImage is implemented — there's a full controller at internal/controller/catalogimage/ (with audit, metrics, publisher) and 6 API routes in internal/buildapi/catalog/routes.go (list, create, get, delete, verify, publish). The parenthetical should be removed.
| - No CatalogImage, ContainerBuild, Workspace, ImageReseal, or Flash coverage (CatalogImage not yet implemented) | |
| - No CatalogImage, ContainerBuild, Workspace, ImageReseal, or Flash coverage |
|
/e2e-operator |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/e2e-test-coverage-proposal.md`:
- Line 206: The coverage target at line 206 states "24 API routes" but the
baseline table earlier in the document counts 46 total routes. Update the "24
API routes" reference in the coverage target line to reflect the correct
baseline count of 46 total routes, ensuring consistency across the entire
proposal and making the 90% coverage target interpretation clear and aligned
with the documented baseline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 25c46b45-1f70-4f7b-9ceb-1b54c205408f
📒 Files selected for processing (1)
test/e2e/e2e-test-coverage-proposal.md
|
|
||
| | Goal | Description | Depends On | | ||
| |------|-------------|------------| | ||
| | Coverage target: 90% | Full coverage of all 7 CRDs, 24 API routes, auth flows, error paths, cleanup, and edge cases. | All phases | |
There was a problem hiding this comment.
Align the long-term API-route target with the baseline table.
Line 206 says “24 API routes,” but the baseline above already counts 46 total routes. That makes the 90% target inconsistent with the rest of the proposal and hard to interpret.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/e2e-test-coverage-proposal.md` at line 206, The coverage target at
line 206 states "24 API routes" but the baseline table earlier in the document
counts 46 total routes. Update the "24 API routes" reference in the coverage
target line to reflect the correct baseline count of 46 total routes, ensuring
consistency across the entire proposal and making the 90% coverage target
interpretation clear and aligned with the documented baseline.
2 participants
Summary
Related Issues
Type of Change
Testing
make test)make lint)make manifests generate)Summary by CodeRabbit
#1–61), and outlines expected CI/workflow trigger changes.