test(e2e): eliminate silent skips; drop global-rule --label#54
Merged
Conversation
Audit of test/e2e/ Skip() sites: 12 of 13 had preconditions already satisfied by the CI environment (skywalking + ai-aws-content-moderation in apisix_conf/config.yaml, stream_proxy enabled, Docker socket reliable, license-gate string is EE-only). Those skips are converted to require.NoError so a regression in any underlying precondition surfaces as a real failure instead of green CI. The 13th skip guarded an APISIX 3.15 limitation: global_rules do not accept labels. Removed the --label flag from a6 global-rule export and delete, along with the related unit tests. TestExport_GlobalRule rewritten with three distinct plugins because APISIX enforces one global_rule per plugin type (a constraint the old test never hit because the labels-skip fired first). Closes #36.
|
Warning Review limit reached
More reviews will be available in 2 minutes and 18 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR removes label-based filtering from global-rule delete and export commands and eliminates conditional skips in multiple e2e tests, replacing them with direct assertions. ChangesGlobal Rule Label Filtering Removal
E2E Test Skip Elimination
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/ssl_stream_route_ginkgo_test.go (1)
1-429:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun gofmt on this file before merge.
CI is already failing on formatting, so this needs a gofmt pass to unblock the branch.
As per coding guidelines,
**/*.go: Follow gofmt and goimports formatting standards.🤖 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/ssl_stream_route_ginkgo_test.go` around lines 1 - 429, The file fails gofmt/goimports formatting; run gofmt (and optionally goimports) on this file to fix formatting issues across functions like indentPEM, readGinkgoTestCert, deleteSSLViaAdminByID, and the Describe test blocks, then re-run CI and commit the formatted file so the build passes.
🤖 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.
Outside diff comments:
In `@test/e2e/ssl_stream_route_ginkgo_test.go`:
- Around line 1-429: The file fails gofmt/goimports formatting; run gofmt (and
optionally goimports) on this file to fix formatting issues across functions
like indentPEM, readGinkgoTestCert, deleteSSLViaAdminByID, and the Describe test
blocks, then re-run CI and commit the formatted file so the build passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 758269b8-ddc1-4cbc-b9e3-0e5811d7412b
📒 Files selected for processing (13)
pkg/cmd/globalrule/delete/delete.gopkg/cmd/globalrule/delete/delete_test.gopkg/cmd/globalrule/export/export.gopkg/cmd/globalrule/export/export_test.gotest/e2e/coverage_flags_test.gotest/e2e/debug_ginkgo_test.gotest/e2e/debug_logs_test.gotest/e2e/export_and_label_test.gotest/e2e/ginkgo_helpers_test.gotest/e2e/scenario_coverage_ginkgo_test.gotest/e2e/skills/skill_plugin_ai_content_moderation_test.gotest/e2e/skills/skill_plugin_skywalking_test.gotest/e2e/ssl_stream_route_ginkgo_test.go
💤 Files with no reviewable changes (7)
- test/e2e/debug_logs_test.go
- test/e2e/scenario_coverage_ginkgo_test.go
- test/e2e/ginkgo_helpers_test.go
- test/e2e/coverage_flags_test.go
- pkg/cmd/globalrule/delete/delete_test.go
- test/e2e/debug_ginkgo_test.go
- pkg/cmd/globalrule/export/export_test.go
There was a problem hiding this comment.
Pull request overview
This PR removes silent skips across the E2E suite so unmet preconditions fail loudly, and it removes the unsupported --label filter from global-rule export/global-rule delete to match APISIX OSS behavior/limitations.
Changes:
- Converted E2E
Skip(...)/t.Skipf(...)guardrails into hard assertions (require.NoError/g.Expect(err).NotTo(HaveOccurred())) across stream-mode, license-gate, plugin availability, and docker-logs checks. - Dropped
--labelsupport frompkg/cmd/globalrule/exportandpkg/cmd/globalrule/delete, and removed the corresponding unit tests. - Updated
TestExport_GlobalRuleE2E coverage to create 3 global rules using distinct plugin types (to satisfy APISIX’s “one global_rule per plugin type” constraint).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/ssl_stream_route_ginkgo_test.go | Removes stream-mode skip helper and enforces failing assertions for stream-route E2E coverage. |
| test/e2e/skills/skill_plugin_skywalking_test.go | Replaces plugin-availability skip with require.NoError to prevent silent regressions. |
| test/e2e/skills/skill_plugin_ai_content_moderation_test.go | Replaces config-rejection skip with require.NoError to prevent silent regressions. |
| test/e2e/scenario_coverage_ginkgo_test.go | Removes license-gate skip so OSS failures are surfaced instead of skipped. |
| test/e2e/ginkgo_helpers_test.go | Removes license-skip helper and now-unused import. |
| test/e2e/export_and_label_test.go | Removes global-rule label-skip path; rewrites global-rule export E2E test to avoid APISIX plugin-type constraint. |
| test/e2e/debug_logs_test.go | Removes auto-detect skip; makes missing docker/container issues fail loudly in E2E. |
| test/e2e/debug_ginkgo_test.go | Removes docker-unavailable skip logic so docker log regressions fail loudly. |
| test/e2e/coverage_flags_test.go | Removes debug-logs pre-skip so debug log flag behavior is always asserted in E2E. |
| pkg/cmd/globalrule/export/export.go | Removes --label flag and label-based filtering from global-rule export. |
| pkg/cmd/globalrule/export/export_test.go | Removes the unit test that validated --label filtering behavior. |
| pkg/cmd/globalrule/delete/delete.go | Removes --label flag and label-based bulk delete path for global rules. |
| pkg/cmd/globalrule/delete/delete_test.go | Removes label-related validation tests for global-rule delete. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
44
to
45
| cmd.Flags().StringVarP(&opts.Output, "output", "o", "yaml", "Output format: json, yaml") | ||
| cmd.Flags().StringVarP(&opts.File, "file", "f", "", "Write output to file") |
Comment on lines
52
to
54
| cmd.Flags().BoolVar(&opts.Force, "force", false, "Skip confirmation prompt") | ||
| cmd.Flags().BoolVar(&opts.All, "all", false, "Delete all global rules") | ||
| cmd.Flags().StringVar(&opts.Label, "label", "", "Delete global rules matching label (key=value)") | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Audit of all
Skip(...)/t.Skipf(...)call sites intest/e2e/per the GA-readiness umbrella (#33).12 of 13 sites had preconditions already satisfied by the CI environment and were masking potential regressions:
scenario_coverage_ginkgo_test.go) — string"requires a sufficient license"is an EE-only response; OSS APISIX never returns it.proxy_mode: http&streamandstream_proxy.tcpare enabled inapisix_conf/config.yaml.skywalkingskill test — plugin enabled by test: enable skywalking plugin in e2e apisix #32.ai-aws-content-moderationskill test — plugin enabled in the same config.apisixthat thename=apisixsubstring filter matches.All converted to
require.NoError/g.Expect(err).NotTo(HaveOccurred())so a regression in any underlying precondition fails CI loudly instead of skipping silently.The 13th site (
export_and_label_test.go:291, global-rule labels) guarded a real APISIX 3.15 limitation —global_rulesrejectslabelswith"additional properties forbidden, found labels". Per issue #36's "remove the feature" branch:--labelflag frompkg/cmd/globalrule/exportandpkg/cmd/globalrule/delete.TestGlobalRuleExport_WithLabelFilter,TestGlobalruleDelete_AllAndLabelMutuallyExclusive,TestGlobalruleDelete_LabelWithID).TestExport_GlobalRulewith three distinct plugins. APISIX OSS enforces oneglobal_ruleper plugin type — the old test never hit this constraint because the labels-skip fired first.Net
-154lines across 13 files. No new code paths, no env-flag gates, no silent skips left in the e2e suite.Test plan
go test ./...— all unit tests greengo test -tags e2e -run 'TestExport_GlobalRule|TestExport_StreamRoute|TestSkillPluginSkywalking|TestSkillPluginAIContentModeration'against a live local APISIX 3.15 — all greengo test -tags e2e -run TestE2E -args -ginkgo.focus='stream-route|scenario coverage'— 7/7 specs passed, 0 skipped within focusgo build ./...andgo vet ./...cleanThe
debug --container apisixtests were not run locally because the local container is named differently; they will pass in CI where the container is namedapisixexactly.Closes #36.
Summary by CodeRabbit
Refactor
Bug Fixes
Tests