Skip to content

Commit e203b75

Browse files
authored
Improve CodeRabbit test execution plans for verified PRs (#5018)
##### What this PR does / why we need it: CodeRabbit test execution plans triggered by the `verified` label were smoke-centric. For utilities or libs changes, plans often stopped at `Run smoke tests: False` and omitted `@pytest.mark.gating` tests reached only through conftest fixtures or teardown (for example HCO golden-image helpers). This PR updates the `verified` workflow comment to CodeRabbit, adds matching `.coderabbit.yaml` path instructions for `utilities/`, `libs/`, and `tests/`, and aligns `TESTS_ORACLE_PROMPT.md`. Plans should now include: - `Run smoke tests` and `Run gating tests` - A required affected-test list for infrastructure changes - Tracing via fixture teardown and `yield from` cleanup, not only direct test imports - ##### Which issue(s) this PR fixes: The verified-label prompt only required a smoke yes/no decision, so utilities changes often produced "Run smoke tests: False" with no gating tests or fixture-teardown paths listed. Extend the workflow comment, CodeRabbit path instructions, and TESTS_ORACLE_PROMPT so plans include gating impact, a concrete affected-test list, and tracing through fixtures and yield-from cleanup. ##### Special notes for reviewer: - No runtime or test code changes. Docs and bot prompt only. - example: #4860, here test plan executions doesn't cover tests affected due to change in teardown/fixture flow ##### jira-ticket: <!-- full-ticket-url needs to be provided. This would add a link to the pull request to the jira and close it when the pull request is merged If the task is not tracked by a Jira ticket, just write "NONE". --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Tightened test-review workflow: first-line inline reviews must include both smoke and gating decisions, justified with concrete dependency paths; added explicit examples for correct/incorrect review content. * **Documentation** * Expanded testing guidance: deeper dependency tracing across imports, shared utilities and test fixtures (including teardown), detection of newly added tests, stricter impact-analysis requirements, and updated recommendations for gating test handling and marking. <!-- review_stack_entry_start --> [![Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/RedHatQE/openshift-virtualization-tests/pull/5018?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Geetika Kapoor <gkapoor@redhat.com>
1 parent fe0e25a commit e203b75

2 files changed

Lines changed: 64 additions & 20 deletions

File tree

.github/workflows/request-coderabbit-test-instructions.yml

Lines changed: 54 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ jobs:
3838
<summary>Test execution plan request details</summary>
3939
4040
CRITICAL: You MUST post an inline review comment on the first changed line of the first file.
41-
The inline comment should contain the Test Execution Plan with the smoke tests recommendation.
41+
The inline comment should contain the full Test Execution Plan (smoke decision, gating decision, and specific affected tests).
4242
Do NOT submit a formal review - just post the inline comment directly.
4343
4444
As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan.
@@ -50,32 +50,67 @@ jobs:
5050
1. Examine code changes in each modified file
5151
2. Identify affected code paths, functions, and classes
5252
3. Analyze pytest-specific elements: fixtures (scope, dependencies), parametrization, markers, conftest changes
53-
4. Trace test dependencies through imports, shared utilities, and fixture inheritance
53+
4. Trace test dependencies through imports, shared utilities, fixture inheritance, fixture teardown, and `yield from` cleanup in conftest
5454
5. Detect new tests introduced in the PR
55-
6. **Smoke Test Impact Analysis:** Determine if any changes could affect smoke tests by checking:
56-
- Changes to files/functions used by tests marked with `@pytest.mark.smoke`
57-
- Changes to fixtures or utilities imported by smoke tests
58-
- Changes to conftest.py files that may affect smoke test execution
59-
- Changes to core infrastructure code (utilities/, libs/) that smoke tests depend on
55+
6. **Utilities and libs impact (when `utilities/` or `libs/` changes):**
56+
You MUST use shell scripts (`rg`, `git diff`) to trace the full impact.
57+
Follow these sub-steps in order:
58+
59+
6a. **Identify modified symbols:** For each changed file under `utilities/` or `libs/`,
60+
list every modified function or method.
61+
Example: `git diff HEAD~1 --unified=0 -- utilities/hco.py | grep '^[+-]def '`
62+
63+
6b. **Find direct callers:** Search tests and conftest for each symbol from 6a.
64+
Example: `rg -l 'get_hco_version' tests/`
65+
66+
6c. **Trace fixture teardown and cleanup:** Find fixtures that reach
67+
the modified symbol through yield from or context-manager wrappers.
68+
Example: `rg -l 'yield from.*enable_common_boot|def.*enable_common_boot' tests/`
69+
70+
6d. **Trace same-file callers:** In each changed file, find other functions
71+
whose body calls a modified symbol (including code after `yield`
72+
in `@contextmanager` helpers).
73+
Example: `rg 'get_hco_version|enable_common_boot' utilities/hco.py`
74+
75+
6e. **Expand transitively:** If function A calls modified B, then
76+
tests/fixtures that call A are affected — even when the test body
77+
never imports B directly.
78+
79+
Do NOT limit impact to tests that import the modified symbol only.
80+
7. **Smoke test impact:** Intersect the affected set from step 6 with smoke-marked tests.
81+
Run: `rg -l '@pytest.mark.smoke' tests/`
82+
Set True ONLY if a smoke-marked file also appears in the affected set from 6b-6e.
83+
8. **Gating test impact:** Intersect the affected set from step 6 with gating-marked tests.
84+
Run: `rg -l '@pytest.mark.gating' tests/`
85+
Set True if a gating-marked file also appears in the affected set from 6b-6e.
86+
Utilities/libs changes often affect gating tests without affecting smoke tests.
87+
Do NOT stop analysis after concluding `Run smoke tests: False`.
88+
89+
**Output rules:**
90+
Do NOT include analysis step numbers (1-8) in your visible output.
6091
6192
**Your deliverable:**
6293
Your change request comment will be based on the following requirements:
6394
6495
**Test Execution Plan**
6596
66-
- **Run smoke tests: True / False** _(Based on your smoke test impact analysis - True if any changes may affect smoke tests)_
97+
- **Run smoke tests: True / False** — If True, state the dependency path (test → fixture → changed symbol). True ONLY with a verified path.
98+
- **Run gating tests: True / False** — If True, state the dependency path. True if any gating-marked test is in the affected set.
99+
- **Affected tests to run** _(required when utilities/, libs/, or shared conftest changes — list concrete paths even when smoke is False)_
67100
- `path/to/test_file.py` - When the entire test file needs verification
68101
- `path/to/test_file.py::TestClass::test_method` - When specific test(s) needed
69102
- `path/to/test_file.py::test_function` - When specific test(s) needed
70-
- `-m marker` - When specific marker(s) can be used to cover multiple cases.
103+
- `-m marker` - When a marker covers multiple affected tests (e.g. `-m gating` only if ALL gating tests in scope need run)
104+
- Tag each listed test or group with its marker when not obvious, e.g. `(gating)` or `(smoke)`
71105
72106
**Guidelines:**
73107
74-
- Include only tests directly affected by the changes
108+
- Include tests affected directly OR via fixture setup/teardown, `yield from` cleanup, or transitive utility call chains (caller calls modified helper)
75109
- Use a full file path only if ALL tests in that file require verification
76-
- Use file path + test name if only specific tests are needed
110+
- Use file path + test name when only specific tests use an affected fixture or utility wrapper (preferred for partial file impact)
77111
- If a test marker can cover multiple files/tests, provide the marker
78112
- Balance coverage vs over-testing - Keep descriptions minimal
113+
- Example: if leaf helper `foo()` changes, include tests whose fixture teardown calls wrapper `bar()` where `bar()` calls `foo()`, even when the test body only imports an unrelated symbol from the same utilities module
79114
80115
**Hardware-Related Checks (SR-IOV, GPU, DPDK):**
81116
@@ -103,14 +138,13 @@ jobs:
103138
104139
**Remember**: The pull request review is visible to users. Additional comments are redundant noise.
105140
106-
CRITICAL: Before setting "Run smoke tests: True", you MUST use shell scripts to:
107-
- Trace the actual fixture dependency chain from smoke tests to changed fixtures
108-
- Verify that smoke tests actually import/use changed utilities or functions
109-
- Confirm the dependency path exists; do NOT assume based on scope or semantics
110-
- Be conservative: Session-scoped fixtures or infrastructure-sounding names do NOT
111-
automatically mean smoke test impact. Only flag smoke test impact when you can
112-
demonstrate a concrete dependency path.
113-
WRONG: "This session-scoped storage fixture might affect smoke tests"
114-
RIGHT: "Smoke test X uses fixture Y, which depends on the changed fixture Z"
141+
CRITICAL — Verification rules for smoke and gating decisions:
142+
- For each True decision, you must have traced a concrete path: test file → fixture → utility function → modified symbol.
143+
- Do NOT assume impact based on scope, file proximity, or infrastructure-sounding names.
144+
Only set True when you trace a concrete path through steps 6a-6e, then 7 or 8.
145+
WRONG: "utilities/hco.py changed → run smoke" (no traced dependency path)
146+
WRONG: "Run smoke tests: False" with no affected-test list when gating tests use the changed utility
147+
RIGHT: "Run smoke tests: False, Run gating tests: True" plus `tests/.../test_foo.py::TestBar::test_baz (gating)`
148+
RIGHT: "Smoke test X uses fixture Y → wrapper Z → changed function W in utilities/hco.py"
115149
116150
</details>

TESTS_ORACLE_PROMPT.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ When possible, recommend a pytest marker (`-m marker_name`) that covers multiple
4545

4646
Common markers in this repository (check `pytest.ini` for the full list):
4747
- `-m smoke` — Smoke tests (critical path validation)
48+
- `-m gating` — Tier2 tests in the gating CI job (often affected by utilities/HCO changes without affecting smoke)
4849
- `-m sriov` — SR-IOV networking tests
4950
- `-m gpu` — GPU passthrough tests
5051
- `-m dpdk` — DPDK networking tests
@@ -57,6 +58,15 @@ If a change affects all tests with a specific marker, recommend `-m marker_name`
5758
When a marker covers ALL affected tests, use: `-m marker_name`
5859
When a marker covers MOST but not all, use both: `-m marker_name` plus individual test paths for the uncovered ones.
5960

61+
### Gating Test Impact Analysis
62+
63+
Determine if any changes could affect gating tests by checking:
64+
- Changes to files/functions used by tests marked with `@pytest.mark.gating`
65+
- Changes to fixtures or utilities imported by gating tests (including fixture teardown and `yield from` cleanup)
66+
- Changes to `utilities/` or `libs/` even when smoke tests are unaffected
67+
68+
Do not stop recommendations when smoke is unaffected. List specific gating test paths or `-m gating` when appropriate.
69+
6070
### Smoke Test Impact Analysis
6171

6272
Determine if any changes could affect smoke tests by checking:

0 commit comments

Comments
 (0)