test(e2e): default the harness to admin-off; held-back tests opt in#800
Conversation
Flip AppOverrides.admin to default false, so the whole suite runs with no admin listener bound — the shape once the Admin API is removed. Only the tests whose subject IS the Admin API surface keep it, opting in with admin: true (a no-op behaviorally — this was the previous default): apikey-budget/-lifecycle, auth-baseline, file-resource-source, health-minimal, prometheus-metrics, openai-sdk-compat, seed-vs-admin-characterization, status-models. Everything else — the majority, already seeding via SeedClient/etcd — now runs admin-off with no per-file change. Scoped by sweeping every case for real admin-listener use (app.adminUrl in a request / AdminClient calls other than listModelStatuses, which reads the metrics listener); the result was exactly those 9 files. Last step of the e2e de-admin-API work (after #791 switch, #792 seed migration): full suite green admin-off proves the Admin API isn't needed to run it, so removal deletes only the admin listener + these 9 tests.
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 59 minutes. |
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe E2E harness now disables the Admin listener by default. Tests that exercise Admin APIs or related endpoints explicitly start applications with ChangesAdmin listener transition
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ 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 |
![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
🧹 Nitpick comments (1)
tests/e2e/src/harness/app.ts (1)
156-156: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the default-off contract.
The changed callers now opt into
admin: true, so add a real-backend harness test that callsspawnApp()without overrides and verifies proxy/metrics readiness while the admin health endpoint remains unavailable. As per coding guidelines, define verifiable success criteria and write validation tests for changed behavior.🤖 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 `@tests/e2e/src/harness/app.ts` at line 156, Add regression coverage for the default-off behavior around spawnApp(), using the real-backend harness without overrides. Verify proxy and metrics become ready while the admin health endpoint remains unavailable, with explicit assertions for each outcome and success criteria covering the default adminEnabled value.Source: Coding guidelines
🤖 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 `@tests/e2e/src/harness/app.ts`:
- Line 156: Update spawnApp’s admin readiness flow so adminEnabled reflects the
effective admin configuration after overrides.extra is merged, preventing
extra.admin.enabled from enabling a listener without the corresponding health
gate. Either reject extra.admin in favor of overrides.admin or derive both
generated configuration and readiness from the same effective admin setting.
---
Nitpick comments:
In `@tests/e2e/src/harness/app.ts`:
- Line 156: Add regression coverage for the default-off behavior around
spawnApp(), using the real-backend harness without overrides. Verify proxy and
metrics become ready while the admin health endpoint remains unavailable, with
explicit assertions for each outcome and success criteria covering the default
adminEnabled value.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a88f9c8-2fba-4b23-b503-c38ede345ff8
📒 Files selected for processing (10)
tests/e2e/src/cases/apikey-budget-e2e.test.tstests/e2e/src/cases/apikey-lifecycle-e2e.test.tstests/e2e/src/cases/auth-baseline-e2e.test.tstests/e2e/src/cases/file-resource-source-e2e.test.tstests/e2e/src/cases/health-minimal-e2e.test.tstests/e2e/src/cases/openai-sdk-compat.test.tstests/e2e/src/cases/prometheus-metrics-e2e.test.tstests/e2e/src/cases/seed-vs-admin-characterization-e2e.test.tstests/e2e/src/cases/status-models-e2e.test.tstests/e2e/src/harness/app.ts
Cold-audit LOW: keep admin:true only on the beforeAll app, whose
`fetch(${app.adminUrl}/metrics)` 404 assertion needs the admin listener
bound; the custom-path and prometheus-false spawns only probe the metrics
port, so drop their redundant admin:true and let them run admin-off. Also
correct the beforeAll comment to state the real reason admin is bound.
|
Independent cold audit (2 lenses — completeness / over-marking + default correctness). PASS, zero blocking. Both lenses converged: the admin-off default flip is correct and complete — exactly the 9 case files that make a real admin-listener request are marked Two LOW nits, both the inverse of a break (admin over-applied in the held-back
Verification: prometheus-metrics 5/5 green after the change (the 2 auxiliary spawns pass admin-off); tsc clean; full suite was 150/150 green on the prior head. |
…ss gate CodeRabbit: `overrides.extra` is spread over the generated config at the top level, so an `extra.admin` would replace the generated admin block and could bind the listener while readiness still keys off `adminEnabled` and skips the admin health gate. Reject `extra.admin` so the `admin` boolean stays the single source of truth for the admin listener.
|
CodeRabbit (readiness vs |
1 participant
What
Flip the e2e harness default to
admin: false, so the whole suite runs with no admin listener bound by default. The only tests that keep the admin listener are the ones whose subject is the Admin API surface — they now opt in explicitly withadmin: true.This is the last step of the e2e "de-admin-API" work (after #791 added the
admin.enabledswitch and #792 migrated the seed-tail tests). It proves the acceptance for retiring the Admin API: the suite is green with the admin listener off by default, so when the Admin API is removed, only the explicitly-admin: truefiles need deletion — everything else already runs admin-off.Changes
tests/e2e/src/harness/app.ts:AppOverrides.adminnow defaults tofalse(overrides.admin ?? false). Doc updated: admin-off is the norm; only held-back admin-surface tests opt in withadmin: true.admin: trueon everyspawnAppsite — a no-op behaviorally (this was the previous default), just making the dependency explicit now that the default flipped:apikey-budget,apikey-lifecycle(admin key rotate / removed-field validation),auth-baseline(admin bearer 401),file-resource-source(file-mode admin write 409 + path-leak),health-minimal/prometheus-metrics(admin/admin/v1/health),openai-sdk-compat(RFC 9745 deprecation-header contract),seed-vs-admin-characterization(seed≡admin equivalence),status-models(/status/modelsvs/admin/v1/models/statusequivalence).Everything else — the large majority of the suite, which already seeds via
SeedClient/etcd and never touches the admin listener — now runs admin-off with no per-file change (the default carries it).Scoping method
Swept every case file for real admin-listener usage (
app.adminUrlin a request /new AdminClientcalling anything other thanlistModelStatuses, which reads the metrics listener). The result was exactly the 9 held-back files above.admin-disabled-e2eand the 9 files migrated in #792 already passadmin: falseexplicitly;status-configseeds via etcd and only references/admin/v1/healthin a comment, so it runs admin-off unchanged.Verification
npx tsc --noEmitclean on the changed files. The 9 held-back files pass unchanged (admin:true = their prior behavior);allowed-modelsandstatus-configconfirm previously-default-admin-on files now run admin-off. Full e2e suite green with the admin-off default — the definitive check that no admin-dependent file was missed (a miss would fail now that the listener is off by default).Follow-up
With this merged, the remaining Admin-API-removal work (AISIX-Cloud#1007 Phase 4) is:
/readyzrelocation + read-surface decision, a v0.4.0 coexistence release, then the removal PR itself (which deletes the admin listener + the 9admin: trueheld-back tests).Summary by CodeRabbit