Add a stats tool to count active and quarantine tests #3000
Conversation
WalkthroughIntroduces a new Python module that builds an HTML dashboard summarizing test statistics. Implements test scanning via AST parsing to detect test functions and quarantine status through decorator analysis, aggregates statistics by category, and generates a self-contained HTML dashboard with summary cards and detailed quarantined tests list. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
stats/generate_dashboard.py (5)
80-84: AddClassVarannotations to mutable class attributes.Per static analysis (RUF012), mutable class attributes should be annotated with
typing.ClassVarto clarify they are shared across instances and not per-instance state.+from typing import ClassVar, Dict, List, NamedTuple, Set + class TestScanner: ... # Folders to exclude from the report - EXCLUDED_FOLDERS = {"after_cluster_deploy_sanity", "deprecated_api"} + EXCLUDED_FOLDERS: ClassVar[Set[str]] = {"after_cluster_deploy_sanity", "deprecated_api"} # Folder mappings (source -> target) for combining stats - FOLDER_MAPPINGS = {"data_protection": "storage", "cross_cluster_live_migration": "storage"} + FOLDER_MAPPINGS: ClassVar[Dict[str, str]] = {"data_protection": "storage", "cross_cluster_live_migration": "storage"}
130-136: Consider narrowing the exception scope or re-raising after logging.The broad
except Exceptioncatches all errors including unexpected ones (e.g.,KeyboardInterruptis not caught, butSystemExitedge cases could be). Based on your preference for fail-fast design, consider either:
- Narrowing to specific expected exceptions (
SyntaxError,OSError,UnicodeDecodeError)- Re-raising after logging if unexpected errors occur
for test_file in test_files: try: tests = self._scan_file(file_path=test_file) all_tests.extend(tests) - except Exception as e: + except (SyntaxError, OSError, UnicodeDecodeError) as e: print(f"Warning: Error scanning {test_file}: {e}")Based on learnings, user prefers fail-fast design.
240-250: Remove unusednoqadirective and consider edge case for root-level test files.
Per static analysis (RUF100),
FCN001is not a valid Ruff code - remove the directive.Edge case: if a test file exists directly in
tests/(e.g.,tests/test_sanity.py),parts[0]would be the filename itself (test_sanity.py), resulting in an unexpected category name.- parts = file_path.relative_to(self.tests_dir).parts # noqa: FCN001 - if len(parts) > 0: + parts = file_path.relative_to(self.tests_dir).parts + if len(parts) > 1: # Require at least one subdirectory category = parts[0] if category in self.EXCLUDED_FOLDERS: return None category = self.FOLDER_MAPPINGS.get(category, category) return category return "uncategorized"
659-659: Remove unusednoqadirective and consider path fragility.
Per static analysis (RUF100), remove the
# noqa: FCN001directive.
Path.cwd()makes the output dependent on the working directory. If someone runs the script from a different directory, the relative paths in the HTML will be incorrect. Consider passing the base path as a parameter or using the project root frommain().- rel_path = test.file_path.relative_to(Path.cwd()) # noqa: FCN001 + rel_path = test.file_path.relative_to(Path.cwd())For the path fragility, consider making
DashboardGeneratoraware of the project root:def __init__(self, stats: DashboardStats, project_root: Path): self.stats = stats self.project_root = project_root
688-688: Remove unusednoqadirective.Per static analysis (RUF100),
FCN001is not a valid Ruff code.- script_dir = Path(__file__).parent # noqa: FCN001 + script_dir = Path(__file__).parent
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
stats/generate_dashboard.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 2469
File: utilities/sanity.py:139-142
Timestamp: 2025-11-08T07:36:57.616Z
Learning: In the openshift-virtualization-tests repository, user rnetser prefers to keep refactoring PRs (like PR #2469) strictly focused on moving/organizing code into more granular modules without adding new functionality, error handling, or behavioral changes. Such improvements should be handled in separate PRs.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1244
File: utilities/os_utils.py:248-250
Timestamp: 2025-07-08T05:51:06.314Z
Learning: User rnetser prefers fail-fast code design where functions should raise exceptions immediately when encountering unexpected or malformed input, rather than implementing defensive error handling with fallbacks. They value making problems visible immediately during development over graceful degradation.
🪛 Ruff (0.14.7)
stats/generate_dashboard.py
1-1: The file is executable but no shebang is present
(EXE002)
81-81: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
84-84: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
134-134: Do not catch blind exception: Exception
(BLE001)
240-240: Unused noqa directive (unknown: FCN001)
Remove unused noqa directive
(RUF100)
659-659: Unused noqa directive (unknown: FCN001)
Remove unused noqa directive
(RUF100)
688-688: Unused noqa directive (unknown: FCN001)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: build-container
- GitHub Check: tox
- GitHub Check: can-be-merged
🔇 Additional comments (9)
stats/generate_dashboard.py (9)
1-18: LGTM - Well-documented module header.The module docstring provides clear usage instructions and configuration documentation.
28-48: LGTM - Clean data model.Appropriate use of
NamedTuplewith well-documented fields and sensible defaults.
50-67: LGTM - Appropriate aggregation structure.
139-205: LGTM - Solid AST-based test detection with class-level quarantine propagation.The two-pass approach (classes first, then functions) correctly handles inheritance of quarantine status from parent classes.
207-222: LGTM - Correct parent class detection.The identity comparison
child is func_nodecorrectly identifies the exact node instance.
252-344: LGTM - Thorough decorator detection with careful scoping.The backward-walk heuristic correctly handles multi-line decorators, and the Jira extraction is properly scoped to the
xfailsection to avoid false positives from@polarionmarkers.
346-376: LGTM - Efficient statistics aggregation.Single-pass iteration with
defaultdictis the right approach.
589-627: LGTM - Clean table row generation with sensible health thresholds.
730-731: LGTM - Standard CLI entry point.
| quarantined_pct = ( | ||
| (self.stats.quarantined_tests / self.stats.total_tests * 100) if self.stats.total_tests > 0 else 0 | ||
| ) | ||
| timestamp = datetime.now().strftime("%Y-%m-%d %H:%M:%S UTC") |
There was a problem hiding this comment.
Timestamp label says "UTC" but uses local time.
datetime.now() returns local time, not UTC. The label will be misleading.
- timestamp = datetime.now().strftime("%Y-%m-%d %H:%M:%S UTC")
+ from datetime import timezone
+ timestamp = datetime.now(timezone.utc).strftime("%Y-%m-%d %H:%M:%S UTC")Or import timezone at the top of the file with the other imports.
📝 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.
| timestamp = datetime.now().strftime("%Y-%m-%d %H:%M:%S UTC") | |
| from datetime import timezone | |
| timestamp = datetime.now(timezone.utc).strftime("%Y-%m-%d %H:%M:%S UTC") |
🤖 Prompt for AI Agents
In stats/generate_dashboard.py around line 408, the timestamp string currently
uses datetime.now() but is labeled "UTC" which is incorrect; change to a true
UTC timestamp by either using datetime.utcnow().strftime("%Y-%m-%d %H:%M:%S
UTC") or, preferably, import timezone and use
datetime.now(timezone.utc).strftime("%Y-%m-%d %H:%M:%S UTC") (add "from datetime
import timezone" to the imports if using timezone).
3 participants
Short description:
Create a tool to scan the tests/ directory for Python test files, and identify quarantined tests, and generates an HTML dashboard with stats on tactive/quaratnined tests
Generated using Claude cli
More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.