UPSTREAM: <carry>: Skip few Mongo tests. #2009
Conversation
WalkthroughIntroduces a centralized skip registry system for flaky e2e tests, with metadata tracking (bug numbers, reasons, overridability). Adds documentation describing the skip mechanism, updates two e2e test suites to invoke skip checks before test execution, and provides utilities to list skipped tests and override skips via environment variable. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mpryc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/e2e/test_skip_config.go (1)
57-75: Consider logging when tests are skipped.The skip mechanism works correctly, but there's no logging to indicate when tests are skipped due to the registry versus when they're skipped due to the environment variable override. Adding a log statement would improve debugging and CI visibility.
Apply this diff to add logging:
+import ( + "fmt" + "log" + "os" + "strings" + + "github.com/onsi/ginkgo/v2" +) func shouldSkipTest(testName string) (bool, string) { skipInfo, exists := testSkipRegistry[testName] if !exists { return false, "" } // Check if skips are disabled via environment variable // Set OADP_SKIP_KNOWN_FAILURES=false to run all tests regardless of skip registry if skipInfo.Overridable { skipEnv := os.Getenv("OADP_SKIP_KNOWN_FAILURES") if strings.ToLower(skipEnv) == "false" { + log.Printf("Test '%s' would be skipped but running due to OADP_SKIP_KNOWN_FAILURES=false", testName) return false, "" } } skipMessage := fmt.Sprintf("SKIPPED: %s - Bug: %s - Reason: %s", testName, skipInfo.BugNumber, skipInfo.Reason) + log.Printf("Skipping test: %s", skipMessage) return true, skipMessage }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
docs/developer/testing/TESTING.md(1 hunks)tests/e2e/backup_restore_cli_suite_test.go(1 hunks)tests/e2e/backup_restore_suite_test.go(1 hunks)tests/e2e/test_skip_config.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (2.5.0)
tests/e2e/test_skip_config.go
[error] 1-1: : found packages e2e (backup_restore_cli_suite_test.go) and e2e_test (test_skip_config.go) in tests/e2e
(typecheck)
🔇 Additional comments (4)
docs/developer/testing/TESTING.md (1)
77-86: Documentation looks good.The documentation clearly explains the skip mechanism, how to override it, and how to add new entries. This will help developers understand and maintain the skip registry.
tests/e2e/backup_restore_cli_suite_test.go (1)
257-259: Skip check integration looks good.The skip check is correctly placed before the flaky-handling logic, ensuring that tests in the skip registry are skipped before any retry attempts are made. The implementation properly retrieves the test name and delegates to the skip mechanism.
tests/e2e/backup_restore_suite_test.go (1)
395-397: Skip check integration looks good.The skip check is correctly implemented and consistent with the CLI test suite. The placement before flaky-handling logic ensures tests are skipped before retry attempts.
tests/e2e/test_skip_config.go (1)
21-53: No alignment issues found—test names in registry match Entry() calls exactly.Verification confirms that all six test entries in the skip registry ("Mongo application CSI", "Mongo application DATAMOVER", "Mongo application BlockDevice DATAMOVER", and their "via CLI" variants) match the corresponding
ginkgo.Entry()descriptions exactly. The skip mechanism will function as intended with these entries.
| @@ -0,0 +1,103 @@ | |||
| package e2e_test | |||
There was a problem hiding this comment.
Fix package name mismatch.
The package declaration e2e_test conflicts with existing test files in the same directory that use package e2e. This will cause a compilation failure.
Apply this diff to fix the package name:
-package e2e_test
+package e2e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| package e2e_test | |
| package e2e |
🧰 Tools
🪛 golangci-lint (2.5.0)
[error] 1-1: : found packages e2e (backup_restore_cli_suite_test.go) and e2e_test (test_skip_config.go) in tests/e2e
(typecheck)
🤖 Prompt for AI Agents
In tests/e2e/test_skip_config.go around line 1 the file declares package
`e2e_test` which conflicts with other test files using package `e2e`; change the
package declaration to `package e2e` so it matches the rest of the test files in
the directory and resolves the compilation failure.
There was a problem hiding this comment.
Not true, look at other files in this folder. They all are in the package e2e_test
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check package declarations in all Go files in tests/e2e/
echo "=== Package declarations in tests/e2e/ directory ==="
fd -e go . tests/e2e/ -x sh -c 'echo "File: {}" && head -1 {} && echo ""'
Length of output: 2029
@mpryc You're absolutely right, and I apologize for the incorrect suggestion. I've verified the package declarations in the tests/e2e/ directory, and all test suite files consistently use package e2e_test:
backup_restore_cli_suite_test.go:package e2e_testtest_skip_config.go:package e2e_testbackup_restore_suite_test.go:package e2e_test- All other test suite files:
package e2e_test
The golangci-lint error claiming that backup_restore_cli_suite_test.go uses package e2e appears to be stale or based on outdated information. The current code is actually correct and consistent. The linter error should resolve after a clean build or when the linter cache is refreshed.
Your package declaration in test_skip_config.go is correct as-is. No changes are needed.
| var testSkipRegistry = map[string]TestSkipInfo{ | ||
| // Mongo CSI and DATAMOVER tests - skipped due to known issues | ||
| "Mongo application CSI": { | ||
| BugNumber: "OADP-XXXX", | ||
| Reason: "CSI snapshots failing for Mongo workloads", | ||
| Overridable: true, | ||
| }, | ||
| "Mongo application DATAMOVER": { | ||
| BugNumber: "OADP-XXXX", | ||
| Reason: "DATAMOVER backup failing for Mongo workloads", | ||
| Overridable: true, | ||
| }, | ||
| "Mongo application BlockDevice DATAMOVER": { | ||
| BugNumber: "OADP-XXXX", | ||
| Reason: "Block device DATAMOVER failing for Mongo workloads", | ||
| Overridable: true, | ||
| }, | ||
| "Mongo application CSI via CLI": { | ||
| BugNumber: "OADP-XXXX", | ||
| Reason: "CSI snapshots failing for Mongo workloads via CLI", | ||
| Overridable: true, | ||
| }, | ||
| "Mongo application DATAMOVER via CLI": { | ||
| BugNumber: "OADP-XXXX", | ||
| Reason: "DATAMOVER backup failing for Mongo workloads via CLI", | ||
| Overridable: true, | ||
| }, | ||
| "Mongo application BlockDevice DATAMOVER via CLI": { | ||
| BugNumber: "OADP-XXXX", | ||
| Reason: "Block device DATAMOVER failing for Mongo workloads via CLI", | ||
| Overridable: true, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Replace placeholder bug numbers with actual bug references.
All entries in the skip registry use placeholder bug numbers "OADP-XXXX" instead of referencing actual bugs. This defeats the purpose of tracking known issues and makes it impossible to verify if tests should still be skipped or if bugs have been resolved.
Update each entry with the actual bug number from your issue tracking system. For example:
"Mongo application CSI": {
- BugNumber: "OADP-XXXX",
+ BugNumber: "OADP-1234",
Reason: "CSI snapshots failing for Mongo workloads",
Overridable: true,
},Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/e2e/test_skip_config.go around lines 21 to 53 the BugNumber fields are
set to the placeholder "OADP-XXXX"; replace each placeholder with the actual
issue/bug tracker ID for the corresponding skip entry (e.g., OADP-1234),
ensuring each TestSkipInfo.BugNumber is the correct authoritative reference from
the issue tracker and update the Reason text if needed to reflect the tracked
bug; commit the updated file so skips can be audited against real bug records.
There was a problem hiding this comment.
@mpryc You're right to question this. If you're planning to fill in the actual bug numbers before merging, or if you're still in the process of filing/identifying the specific issues, placeholder values are perfectly fine for now.
The main value of actual bug numbers is being able to track when issues are resolved and tests can be re-enabled, but if that's something you're planning to add later or handle differently, that works too. I'll leave it to your judgment on what makes sense for this PR.
- Introduce mechanism to easilly skip tests. - Added few Mongo tests to be skipped. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Michal Pryc <mpryc@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/e2e/test_skip_config_test.go (2)
21-53: Consider sorting the output inGetSkippedTestsListfor deterministic results.The
testSkipRegistryis a map, and Go maps have non-deterministic iteration order. WhenGetSkippedTestsList()iterates through this map (lines 94-101), the output order will vary between runs, which can make CI output and documentation inconsistent.To ensure consistent output, sort the test names before formatting:
func GetSkippedTestsList() string { if len(testSkipRegistry) == 0 { return "No tests are currently skipped" } // Collect and sort test names for deterministic output testNames := make([]string, 0, len(testSkipRegistry)) for testName := range testSkipRegistry { testNames = append(testNames, testName) } sort.Strings(testNames) var sb strings.Builder sb.WriteString("Currently skipped tests:\n") for _, testName := range testNames { skipInfo := testSkipRegistry[testName] sb.WriteString(fmt.Sprintf(" - %s\n", testName)) sb.WriteString(fmt.Sprintf(" Bug: %s\n", skipInfo.BugNumber)) sb.WriteString(fmt.Sprintf(" Reason: %s\n", skipInfo.Reason)) if skipInfo.Overridable { sb.WriteString(" Override: Set OADP_SKIP_KNOWN_FAILURES=false to run this test\n") } } return sb.String() }Don't forget to add
"sort"to the imports.
19-53: Consider adding a mechanism to validate registry keys match actual tests.The skip registry relies on exact string matching between the keys here and the
ginkgo.Entry()descriptions in test files. If a test name changes or there's a typo in the registry, the skip will silently fail with no warning.Consider one of these approaches:
- Define test name constants to use in both the registry and Entry descriptions:
const ( TestMongoCSI = "Mongo application CSI" TestMongoCSIViaCLI = "Mongo application CSI via CLI" // ... etc )
Add a utility function that can be called in tests to log a warning if a registry key doesn't match any actual test (though this requires test discovery).
Document the coupling in comments to remind maintainers that test names must match exactly.
At minimum, add a comment to the registry explaining that keys must exactly match the test Entry descriptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
docs/developer/testing/TESTING.md(1 hunks)tests/e2e/backup_restore_cli_suite_test.go(1 hunks)tests/e2e/backup_restore_suite_test.go(1 hunks)tests/e2e/test_skip_config_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/backup_restore_suite_test.go
- docs/developer/testing/TESTING.md
🔇 Additional comments (3)
tests/e2e/test_skip_config_test.go (2)
55-75: The skip logic is well-implemented.The
shouldSkipTestfunction correctly handles registry lookups, environment variable overrides, and message formatting. The case-insensitive check for the environment variable is a good touch.
77-83: LGTM! Clean wrapper for the skip mechanism.The
SkipIfNeededfunction provides a clean, simple API for tests to integrate the skip functionality.tests/e2e/backup_restore_cli_suite_test.go (1)
257-259: Skip integration is correctly implemented.The skip check is properly placed before test execution, and the use of
CurrentSpecReport().LeafNodeTextshould return the Entry description which matches the registry keys defined intest_skip_config_test.go.For the Mongo tests being skipped, verify the Entry descriptions match the registry keys:
- Line 277: "Mongo application CSI via CLI" ✓
- Line 310: "Mongo application DATAMOVER via CLI" ✓
- Line 332: "Mongo application BlockDevice DATAMOVER via CLI" ✓
kaovilai
left a comment
kaovilai
left a comment
There was a problem hiding this comment.
IMO, test entries to skip should be marked as skip using PEntry vs Entry
https://onsi.github.io/ginkgo/#pending-specs
|
@mpryc: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Nice, wasn't aware of Pending. Does it allow to run those Pending? E.g. we want to have job running every a while to test if they are fixed ? In this PR implementation it's just one ENV variable to enable skipped ones. |
you can unmark single cases as no longer pending in a pr to test that specific one. you can alternatively focus on a test to only test that one. |
|
FEntry would focus the test to only run this specific one.. but would only work if ginkgo run does not also --filter.label (approx flag name) which we do. |
openshift-ci
bot
added
the
do-not-merge/hold
|
closing as not needed per #2013 |
2 participants
Slightly different approach to disable some tests.
#2002