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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughThis change adds documentation and configuration for a new OpenShift plugin command Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (6 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
| "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?
| - 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
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
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