feat: add test resource collector utility for debugging test failures - #1034
feat: add test resource collector utility for debugging test failures#1034fabikova wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThe test suite adds optional Kubernetes resource collection. A pytest option enables the feature, and an autouse fixture invokes collection after each test. Matching resources are discovered, filtered, sanitised, and written as YAML. ChangesResource collection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Pytest
participant collect_resources
participant KubernetesAPI
participant DebugResources
Pytest->>collect_resources: invoke after each test
collect_resources->>KubernetesAPI: discover and retrieve resources
collect_resources->>DebugResources: write matching resources as YAML
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description explains the purpose, lists the main features, documents usage, and provides verification commands and results. It does not use the exact template headings for Description and Changes, but it contains the required information and is sufficiently complete. ✨ 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 |
d23c7d6 to
68b7dfe
Compare
![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 (2)
testsuite/tests/conftest.py (1)
462-466: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the fixture docstring with the actual behaviour.
The fixture is function-scoped, so it runs after each test, not after all tests in the module. The collector then skips repeated calls. State this in the docstring so the scope choice stays clear to maintainers.
♻️ Proposed docstring change
`@pytest.fixture`(scope="function", autouse=True) def collect_test_resources(request, module_label): - """Collect module resources after all tests in the module finish.""" + """Collect module resources after each test. + + The scope is function so that teardown runs before module-scoped fixtures + delete the resources. The collector skips repeated collection per module. + """ yield collect_resources(request.node, module_label)🤖 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 `@testsuite/tests/conftest.py` around lines 462 - 466, Update the docstring of collect_test_resources to state that it collects module resources after each test, with repeated calls skipped by the collector.testsuite/utils/resource_collector.py (1)
129-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the failure when the
getcall does not succeed.If
result.status()is not 0, the code writes a file that states "No matching resources found". A user cannot distinguish an empty result from a failed query. The singlegetwith all discovered types is also likely to fail on large clusters, because one unknown or forbidden type makes the whole call fail.Log the non-zero status and the stderr output.
♻️ Proposed change to report query failures
result = invoke("get", [",".join(resource_types), "--ignore-not-found", "-n", project, "-o", "yaml"]) - if result.status() == 0 and result.out().strip(): + if result.status() != 0: + logger.warning("Resource query failed with status %s: %s", result.status(), result.err()) + elif result.out().strip():🤖 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 `@testsuite/utils/resource_collector.py` around lines 129 - 140, Update the resource query handling around invoke and result.status() to log non-zero query failures, including the status code and result.err() output, before continuing to write the no-matching-resources result. Preserve the existing successful YAML parsing and matching behavior.
🤖 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 `@testsuite/utils/resource_collector.py`:
- Around line 74-75: Sanitize both file-name components used to construct
base_filename in the resource collection helper, especially item.name, replacing
path separators and other unsafe characters with a safe substitute before
building the debug-resources path. Preserve readable uniqueness for parametrized
test IDs and ensure the resulting path can be opened without triggering the
warning-handling path.
- Around line 46-57: Update the de-duplication logic in the resource collection
function to check and store the full module_label in _collected_modules. Keep
base_pattern calculation for resource metadata matching, but pass base_pattern
only to _save_matching_resources (or the equivalent resource-matching helper),
not as the collected-module key.
---
Nitpick comments:
In `@testsuite/tests/conftest.py`:
- Around line 462-466: Update the docstring of collect_test_resources to state
that it collects module resources after each test, with repeated calls skipped
by the collector.
In `@testsuite/utils/resource_collector.py`:
- Around line 129-140: Update the resource query handling around invoke and
result.status() to log non-zero query failures, including the status code and
result.err() output, before continuing to write the no-matching-resources
result. Preserve the existing successful YAML parsing and matching behavior.
🪄 Autofix
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 Plus
Run ID: 59ee4655-8c50-4338-afe5-0466da9562ab
📒 Files selected for processing (2)
testsuite/tests/conftest.pytestsuite/utils/resource_collector.py
b6f832f to
b0846ae
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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 `@testsuite/utils/resource_collector.py`:
- Around line 54-58: Update collect_resources() and _save_resources() so
_save_resources() returns an explicit success result and propagates Kubernetes
query, yaml.YAMLError, and file-writing failures without writing the no-match
marker. Record module_label in _collected_modules only after _save_resources()
successfully completes discovery, querying, parsing, and output writing;
preserve the no-match marker only for successful queries with no matching
resources.
🪄 Autofix
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 Plus
Run ID: 0e0db25f-bb24-4c3f-b529-6ff22c006063
📒 Files selected for processing (2)
testsuite/tests/conftest.pytestsuite/utils/resource_collector.py
🚧 Files skipped from review as they are similar to previous changes (1)
- testsuite/tests/conftest.py
b0846ae to
602022b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
testsuite/utils/resource_collector.py (1)
95-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the pylint suppression at the collection boundary.
except Exceptionmaps configuration, Kubernetes, YAML, and filesystem failures to one warning. Catch expected failures at the smallest scope. Retain a final broad catch only if the optional collector must never fail the test run, and document that boundary.As per coding guidelines, always look for a more correct solution before disabling a pylint warning.
🤖 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 `@testsuite/utils/resource_collector.py` around lines 95 - 97, Refine the exception handling in the resource collection function around the broad `except Exception`: catch expected configuration, Kubernetes, YAML, and filesystem exceptions at their smallest applicable scopes, removing the broad pylint suppression. Retain a final broad catch only if the optional collector must not fail tests, and document that boundary clearly.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 `@testsuite/utils/resource_collector.py`:
- Around line 61-66: The _discover_resource_types and _save_resources flow must
avoid combining forbidden resource types into one list request. Check list
permission for each discovered type before constructing the combined query, or
invoke each type independently, while preserving successful collection for
permitted types and ensuring denied types do not prevent the resource file from
being saved.
---
Nitpick comments:
In `@testsuite/utils/resource_collector.py`:
- Around line 95-97: Refine the exception handling in the resource collection
function around the broad `except Exception`: catch expected configuration,
Kubernetes, YAML, and filesystem exceptions at their smallest applicable scopes,
removing the broad pylint suppression. Retain a final broad catch only if the
optional collector must not fail tests, and document that boundary clearly.
🪄 Autofix
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 Plus
Run ID: cbf38e81-0aad-4376-9dfa-daf224681be9
📒 Files selected for processing (2)
testsuite/tests/conftest.pytestsuite/utils/resource_collector.py
🚧 Files skipped from review as they are similar to previous changes (1)
- testsuite/tests/conftest.py
Signed-off-by: Martina Fabikova <mfabikov@redhat.com>
602022b to
0761264
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Signed-off-by: Martina Fabikova <mfabikov@redhat.com>
Signed-off-by: Martina Fabikova <mfabikov@redhat.com>
1 participant
Add a utility to collect and save Kubernetes resources created during test runs for debugging purposes. When enabled via
--collect-resourcesflag, the collector captures all test-related resources and saves them todebug-resources/as clean YAML files.Features:
Enabled via pytest flag:
poetry run pytest --collect-resources
Apply file (spec-only):
Full file (debugging):
Verification:
Singlecluster test:
poetry run pytest testsuite/tests/singlecluster/gateway/authpolicy/test_authpolicy_section_targeting_gateway.py --collect-resources
→ Two files created (-apply.yaml and -full.yaml)
Parametrized tests:
poetry run pytest testsuite/tests/singlecluster/authorino/identity/api_key/test_auth_credentials.py --collect-resources
→ 16 parameter variants × 2 files = 32 files created (each with different AuthPolicy)
Design:
Future Work
Phase 2: Automated collection for CI failures
Implement automated collection and Report Portal integration:
testsuite
--collect-resources-on-failureflag (collect only failed tests)$WORKSPACE/debug-resources/(Tekton shared workspace)testsuite-rptool
rptool attach --dir debug-resources/afterwritesteptestsuite-pipelines
Result: Failed test in RP → Logs tab → downloadable YAML
Uses Tekton workspace (like JUnit XML already does) — YAML persists on shared PVC between tasks.
This addresses issue #861 by providing an automated way to collect test resources without manually stopping tests and copying resources.