Add a command for reviewing e2e test PRs and enforcing conventions #186
Conversation
…mpat with openshift CI
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughAdds documentation for a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touchesImportant Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (6 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (2)
Comment |
openshift-ci
bot
added
the
approved
dgoodwin
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
plugins/openshift/commands/test-review.md (3)
191-194: Remove redundant "only" adverb.Lines 191-192 repeat "only" in adjacent bullet points, creating a minor style issue.
- - Test operates only in its own namespace + - Test operates in its own namespace - Creates only namespaced resources (pods, services, deployments, etc.)
10-10: Specify language identifiers for fenced code blocks.Multiple code blocks lack language specifiers, which prevents syntax highlighting. Add appropriate language identifiers (
bash,go,markdown, etc.) to all fenced code blocks for better readability.Examples:
- Line 10:
```bash(instead of```)- Lines 261+:
```bashfor command examples- Line 335+:
```gofor Go code violations and fixes- Line 433+:
```markdownfor example outputAlso applies to: 261-261, 266-266, 271-271, 276-276, 281-281, 433-433
431-432: Convert bare URLs to markdown links.Bare URLs should be wrapped in markdown link syntax for better formatting and clickability in rendered documentation.
- - See official OpenShift feature development guidelines: https://github.com/openshift/enhancements/blob/master/dev-guide/feature-zero-to-hero.md - - See test naming guidelines: https://github.com/openshift/origin/blob/master/test/extended/README.md + - See [official OpenShift feature development guidelines](https://github.com/openshift/enhancements/blob/master/dev-guide/feature-zero-to-hero.md) + - See [test naming guidelines](https://github.com/openshift/origin/blob/master/test/extended/README.md)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
PLUGINS.md(1 hunks)docs/data.json(1 hunks)plugins/openshift/commands/test-review.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
plugins/openshift/commands/test-review.md
[style] ~192-~192: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ... only in its own namespace - Creates only namespaced resources (pods, services, d...
(ADVERB_REPETITION_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
plugins/openshift/commands/test-review.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
261-261: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
266-266: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
271-271: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
276-276: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
281-281: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
335-335: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
431-431: Bare URL used
(MD034, no-bare-urls)
432-432: Bare URL used
(MD034, no-bare-urls)
433-433: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (5)
PLUGINS.md (1)
206-206: Documentation entry is well-placed and consistent.The new command entry follows the established pattern and integrates seamlessly with existing OpenShift plugin documentation.
docs/data.json (1)
645-650: JSON command metadata is well-formed and consistent.The new command entry follows the standard structure of other openshift plugin commands, with all required fields properly populated.
plugins/openshift/commands/test-review.md (3)
1-50: Comprehensive implementation guide with strong coverage of PR objectives.The documentation thoroughly addresses all stated PR objectives: component mapping validation, serial test detection, PR/branch review support, integration with OpenShift guidelines (zero-to-hero), and detection of deprecated conventions (Owner, LEVEL0). The 9-step implementation plan provides clear procedural guidance, and the validation rules with examples are well-explained.
24-229: Implementation plan is thorough and practical.The step-by-step breakdown covers change detection, test identification, validation logic, and reporting clearly. The approach to handling PR mode vs. branch mode is sensible, and the validation rules for component tags, naming violations, and parallel safety are comprehensive and aligned with OpenShift testing practices.
258-433: Examples and expected output effectively demonstrate command behavior.The example scenarios (from simple auto-detection to detailed PR analysis) and the sample report with violations clearly show how the command surfaces issues. The violation examples with code snippets and suggested fixes provide practical guidance for developers.
|
|
||
| 1. **Load OpenShift Testing Guidelines**: | ||
| - Fetch the latest guidelines for landing a feature in OpenShift from the enhancements repository | ||
| - Use `curl` to download: `https://raw.githubusercontent.com/openshift/enhancements/refs/heads/master/dev-guide/feature-zero-to-hero.md` |
There was a problem hiding this comment.
nit - Add a timeout recommendation (e.g., curl --connect-timeout 5 --max-time 10)
There was a problem hiding this comment.
IMO this could be a simple (in line?) bash script the command uses.
Will avoid the set of problems where claude decides to do something slightly differently.
There was a problem hiding this comment.
From an AI code review plugin -
File: plugins/openshift/commands/test-review.md
Issue: There is functional overlap with existing commands:
- openshift:review-test-cases - Reviews test cases for quality and best practices
- openshift:expand-test-case - Expands test ideas into comprehensive scenarios
The new test-review command focuses on naming/structure violations specifically, which is distinct, but the naming could cause confusion.
Recommendation: Consider:
1. Adding a "See Also" section referencing related commands
2. Clarifying the distinction in the Description section
|
This looks great :D I've been working on a framework for evals for CC commands like this, starting with the |
|
|
||
| 1. **Load OpenShift Testing Guidelines**: | ||
| - Fetch the latest guidelines for landing a feature in OpenShift from the enhancements repository | ||
| - Use `curl` to download: `https://raw.githubusercontent.com/openshift/enhancements/refs/heads/master/dev-guide/feature-zero-to-hero.md` |
There was a problem hiding this comment.
That doc seems more high level and I'm not sure how you would evaluate many of these things against a single origin PR. It has no knowledge of how many existing tests there are for a feature, or how often they run.
https://github.com/openshift/enhancements/blob/master/dev-guide/test-conventions.md seems more germane
There was a problem hiding this comment.
That does look better, I'll switch it.
docs/data.json
Outdated
| "argument_hint": "[base-branch-or-pr-url]", | ||
| "description": "Review Ginkgo test code changes in current branch or PR for naming violations and best practices", | ||
| "name": "test-review", |
There was a problem hiding this comment.
Is a PR the right argument? What if I wanted to review an existing test that had already merged?
There was a problem hiding this comment.
Post merge review of tests wasn't really an intended target, I thought it would be useful while working locally, and on posted PRs (which I think actually should be the main one once it's analyzing test results coming out of a PR). However you should be able to point it to a PR where tests merged I think.
| - Run `git grep -r "\[sig-<name>\]" test/` or `git grep -r "\[bz-<name>\]" test/` | ||
| - If tag exists in other tests, it's valid | ||
| - If tag is unique to this test, flag as potential made-up tag | ||
| - Examples: `[sig-network]`, `[bz-storage]` |
There was a problem hiding this comment.
I'd avoid mentioning the bugzilla prefix, we should be working on getting rid of this
There was a problem hiding this comment.
If we do though, it will start to flag edits to those tests I think, and then we need to get it renamed in the other repo. I'll try to clarify that it's ok in old tests, but not in new.
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
|
Command isn't done but I've expanded more. Eventually I want it studying your test results looking for problems you may have caused with a PR, and reocommending intelligent additional test jobs to run if the pr looks like it might break microshift/hypershift/etc. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
plugins/ci/commands/pr-test-analysis.md (2)
218-218: Consider removing the repeated "only" for clarity.The sentence "Test operates only in its own namespace - Creates only namespaced resources" uses "only" twice. Consider: "Test operates exclusively in its own namespace - Creates only namespaced resources" or "Test operates only in its own namespace - Creates namespaced resources".
1-659: Consider adding a "See Also" section.Per previous review feedback, adding a "See Also" section would help clarify the relationship with related commands and avoid confusion:
## See Also Related commands for test development and review: - `/openshift:review-test-cases` - Review test cases for completeness, quality, and best practices - `/openshift:expand-test-case` - Expand test ideas into comprehensive test scenarios - `/openshift:new-e2e-test` - Write and validate new OpenShift E2E tests using Ginkgo This command (`ci:pr-test-analysis`) focuses specifically on validating test code changes in PRs and branches against OpenShift naming conventions, component tagging requirements, and parallel execution safety standards.This would address the functional overlap concern raised in the previous review.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
PLUGINS.md(1 hunks)docs/data.json(1 hunks)plugins/ci/README.md(1 hunks)plugins/ci/commands/pr-test-analysis.md(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Lint Plugins
docs/data.json
[error] 1-1: CLAUDELINT: docs/data.json is out of sync with plugin metadata. Run 'make update' to update.
PLUGINS.md
[error] 1-1: CLAUDELINT: PLUGINS.md is out of sync with plugin metadata. Run 'make update' to update.
🪛 LanguageTool
plugins/ci/commands/pr-test-analysis.md
[style] ~218-~218: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ... only in its own namespace - Creates only namespaced resources (pods, services, d...
(ADVERB_REPETITION_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
plugins/ci/commands/pr-test-analysis.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
273-273: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
274-274: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
275-275: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
276-276: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
277-277: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
278-278: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
279-279: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
280-280: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
281-281: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
282-282: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
282-282: Bare URL used
(MD034, no-bare-urls)
283-283: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
284-284: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
285-285: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
286-286: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
287-287: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
288-288: Unordered list indentation
Expected: 4; Actual: 7
(MD007, ul-indent)
289-289: Unordered list indentation
Expected: 4; Actual: 7
(MD007, ul-indent)
290-290: Unordered list indentation
Expected: 4; Actual: 7
(MD007, ul-indent)
330-330: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
335-335: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
340-340: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
345-345: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
350-350: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
427-427: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
555-555: Bare URL used
(MD034, no-bare-urls)
556-556: Bare URL used
(MD034, no-bare-urls)
579-579: Bare URL used
(MD034, no-bare-urls)
580-580: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (5)
plugins/ci/README.md (1)
9-48: LGTM!The documentation is clear, comprehensive, and well-organized. The distinction between NEW and MODIFIED test requirements is well-explained, and the examples cover the main usage scenarios effectively.
plugins/ci/commands/pr-test-analysis.md (4)
1-28: Clear and well-structured command description.The description effectively outlines the four main validation types and clearly distinguishes between requirements for NEW tests versus MODIFIED tests. The scope (Ginkgo tests in OpenShift repos) is appropriate.
292-326: LGTM!The report format is well-structured with clear, logical sections that make it easy to review findings by category.
327-580: Excellent comprehensive examples.The example invocations cover all major use cases, and the detailed sample output effectively demonstrates the report format and how violations are presented with suggested fixes.
582-659: Comprehensive documentation with clear prerequisites and notes.The Arguments, Prerequisites, and Notes sections are thorough and address important edge cases. The explanation of NEW vs MODIFIED test detection and the clarification about fmt.Sprintf usage with string literals are particularly helpful.
5 participants
Goal is to help maintain consistency in openshift e2e testing, prevent problems that will cause prs to get reverted or trouble for CI systems, and ultimately land features smoother.
Enforces a number of conventions that have a history of tripping people up and recommends solutions, mostly around test naming.
This is just a start, hoping to expand on this as we see problems in CI.
It pulls in Bryce's dev guide early to get the context of how we land features and attempt to use that when reviewing.
Works on both local git branch, as well as a PR URL.
A potential next step would be to examine all test results coming out of a PR to see if a modified/added test failed anywhere, or potentially, caused something else to fail around the time the test ran.
Sample output:
Test Review Summary - PR #30539
Overview
Violations Found
❌ Component Mapping (2 violations)
Both tests missing
[Jira:"component"]tag - required for failure routing✅ Parallel Safety (0 violations)
Tests correctly marked
[Serial]- they scale down operators and modify cluster resources✅ Naming Conventions (0 violations)
No dynamic content or deprecated conventions found
Action Items
Add component tag to both tests:
Verify feature has ≥5 tests total (OpenShift requirement, currently 2)
Confirm correct Jira component using:
Test Names
[sig-storage][OCPFeature:MutableCSINodeAllocatableCount][Serial][Driver: ebs.csi.aws.com] should automatically update CSINode allocatable count when instance attached ENI count changes[sig-storage][OCPFeature:MutableCSINodeAllocatableCount][Serial][Driver: ebs.csi.aws.com] should immediately update CSINode allocatable count when ResourceExhausted errors occurReferences
Summary by CodeRabbit
/ci:pr-test-analysis [base-branch-or-pr-url]command to review Ginkgo test code changes for naming, component tagging, topology and parallel-safety guidance, with examples and usage modes./openshift:bootstrap-omcommand signature display by removing an unnecessary empty-string argument.✏️ Tip: You can customize this high-level summary in your review settings.