STD placeholder stats: scan and report unimplemented test placeholders#4058
Open
rnetser wants to merge 8 commits intoRedHatQE:mainfrom
Open
STD placeholder stats: scan and report unimplemented test placeholders#4058rnetser wants to merge 8 commits intoRedHatQE:mainfrom
rnetser wants to merge 8 commits intoRedHatQE:mainfrom
Conversation
…zation-tests into feat/std-placeholder-stats
…zation-tests into feat/std-placeholder-stats
Contributor
📝 WalkthroughWalkthroughAdds a flake8 exclude pattern to pre-commit, introduces a new CLI tool that discovers and reports STD placeholder tests (modules/classes/functions marked Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
…zation-tests into feat/std-placeholder-stats
…ve structure - Replace argparse CLI with click commands and options - Introduce PlaceholderClass and PlaceholderFile dataclasses - Consolidate four AST visitor functions into a single unified function - Split large functions and extract constants for readability - Add error handling for file parsing and output operations - Improve test coverage with additional edge case tests
…zation-tests into feat/std-placeholder-stats
rnetser
changed the title
Contributor
|
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-3
added
branch-main
size/XXL
and removed
needs-rebase
labels
rnetser
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.pre-commit-config.yaml:
- Line 49: The current pre-commit "exclude" regex is too broad (the exclude key
with "(utilities/unittests/|utilities/junit_ai_utils\.py|scripts/.*/tests/)")
and removes linting for entire directories; change it to exclude only the
specific files that need E402 handling by replacing the directory patterns with
file-level or narrowly scoped regexes (e.g., target exact filenames under
utilities/unittests and exact script test paths) so the exclude remains limited
to those unique files; update the exclude value to list only the necessary file
paths (keeping utilities/junit_ai_utils\.py) and remove the blanket
utilities/unittests/ and scripts/.*/tests/ directory-wide patterns.
In `@scripts/std_placeholder_stats/std_placeholder_stats.py`:
- Around line 217-218: The current prefilter checks for the exact substring
f"{TEST_ATTR} = False" in file_content and will miss valid variants like
"__test__=False" or other whitespace/comments; update the prefilter that uses
TEST_ATTR and file_content to detect assignments more robustly by matching a
regex for TEST_ATTR\s*=\s*False (or use AST parsing to look for an ast.Assign to
a Name with id == TEST_ATTR and a Constant False) so all whitespace variants are
accepted before deciding to continue; modify the conditional around that check
(the block using TEST_ATTR and file_content) to use the regex/AST check instead.
- Around line 214-216: The except blocks that catch (UnicodeDecodeError,
OSError) and the similar block later currently log and continue, which swallows
malformed inputs; instead, after logging include context and re-raise the
original exception to fail fast — update the handlers that reference LOGGER and
test_file to call LOGGER.warning with the contextual message and then do "raise"
(re-raising the caught exc to preserve traceback) so processing stops on
read/parse errors rather than continuing silently.
In `@scripts/std_placeholder_stats/tests/test_std_placeholder_stats.py`:
- Around line 507-530: Add two tests that exercise the UnicodeDecodeError and
OSError branches in scan_placeholder_tests: create one test file with
undecodable bytes (e.g., write bytes that will raise UnicodeDecodeError when
opened as text) and assert it is skipped like the SyntaxError case; create
another test that simulates an OSError when reading (e.g., remove read
permissions or mock open to raise OSError) and assert scan_placeholder_tests
skips that file as well. Use the existing helper _create_test_file and the test
harness pattern in test_handles_syntax_errors_gracefully and reference
scan_placeholder_tests and the file path assertions to verify the
unreadable/undecodable files are not included while valid test files are.
- Around line 623-630: The test currently forces logger.propagate to False in
the finally block which can leak state; modify the test around the logger setup
(the logger variable created with
logging.getLogger("scripts.std_placeholder_stats.std_placeholder_stats")) to
save the original value (e.g., original_propagate = logger.propagate) before
changing it, set logger.propagate = True for the test, and in the finally block
restore logger.propagate = original_propagate so the logger's propagate state is
returned to its prior value after calling output_text(...) under caplog; apply
the same pattern to the other similar block around lines 639-645.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 219ee2cc-86cc-4d80-a5ad-0835784a3da7
📒 Files selected for processing (5)
.pre-commit-config.yamlscripts/std_placeholder_stats/__init__.pyscripts/std_placeholder_stats/std_placeholder_stats.pyscripts/std_placeholder_stats/tests/__init__.pyscripts/std_placeholder_stats/tests/test_std_placeholder_stats.py
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
… exclusion, add error handling tests
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
scripts/std_placeholder_stats/tests/test_std_placeholder_stats.py (1)
507-573:⚠️ Potential issue | 🟡 MinorMEDIUM: Error-path tests are still partially non-deterministic
Line 550 uses
chmod(0o000)to triggerOSError, which can be unreliable on privileged runners, and this block still does not explicitly exercise theUnicodeDecodeErrorpath. Prefer deterministic branch forcing (mockPath.read_textforOSError, and write undecodable bytes for decode failure).Deterministic coverage pattern
+ def test_handles_unicode_decode_errors_gracefully(self, tests_dir: Path) -> None: + """scan_placeholder_tests() skips undecodable files and continues.""" + undecodable_path = tests_dir / "test_bad_encoding.py" + undecodable_path.write_bytes(b"\xff\xfe\xfa") + _create_test_file( + directory=tests_dir, + filename="test_valid.py", + content=f'{TEST_FALSE_MARKER}\n\nclass TestGood:\n def test_pass(self):\n """Placeholder test."""\n', + ) + + result = scan_placeholder_tests(tests_dir=tests_dir) + file_paths = [placeholder_file.file_path for placeholder_file in result] + assert "tests/test_bad_encoding.py" not in file_paths + assert "tests/test_valid.py" in file_paths🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/std_placeholder_stats/tests/test_std_placeholder_stats.py` around lines 507 - 573, The test_handles_unreadable_files_gracefully test is non-deterministic because it uses unreadable.chmod(0o000) to provoke an OSError and doesn't exercise UnicodeDecodeError; instead, replace the chmod approach by mocking Path.read_text (or scan_placeholder_tests' internal file read call) to raise OSError for the unreadable path and create a second fixture file that contains bytes not decodable as UTF-8 to trigger UnicodeDecodeError when read; update assertions to ensure scan_placeholder_tests ignores both failing files and still returns the readable file (reference test_handles_unreadable_files_gracefully, scan_placeholder_tests, and Path.read_text to locate where to apply the mock and where to create the undecodable bytes file), and remove any chmod-based permission manipulation so cleanup is deterministic..pre-commit-config.yaml (1)
49-49:⚠️ Potential issue | 🟠 MajorHIGH: Flake8 exclusion is still too broad at directory scope
Line 49 excludes whole directories (
utilities/unittests/,scripts/std_placeholder_stats/tests/), which weakens lint coverage for unrelated/new files in those trees. Keep excludes file-scoped to only the known exceptions.Suggested narrowing
- exclude: "(utilities/unittests/|utilities/junit_ai_utils\\.py|scripts/std_placeholder_stats/tests/)" + exclude: "(utilities/unittests/test_oadp\\.py|utilities/junit_ai_utils\\.py|scripts/std_placeholder_stats/tests/test_std_placeholder_stats\\.py)"Based on learnings, only specific files in
utilities/unittestsrequire targeted E402 handling; broad directory exclusion is not required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.pre-commit-config.yaml at line 49, The flake8 exclude pattern under the exclude key currently removes entire directories via the pattern "(utilities/unittests/|utilities/junit_ai_utils\.py|scripts/std_placeholder_stats/tests/)" — narrow this to file-scoped excludes instead: remove the directory-wide fragments ("utilities/unittests/" and "scripts/std_placeholder_stats/tests/") and replace them with explicit file patterns for the few known exceptions (e.g. the specific test files in utilities/unittests that need E402 and the exact files under scripts/std_placeholder_stats/tests/), keeping utilities/junit_ai_utils\.py as-is; update the exclude string to list only those exact filenames so unrelated/new files are still linted.scripts/std_placeholder_stats/std_placeholder_stats.py (1)
147-155:⚠️ Potential issue | 🔴 CriticalCRITICAL: Placeholder detection currently accepts
docstring + passLine 147-Line 155 classifies
docstring + passas placeholder, which breaks STD semantics (“docstring-only and no implementation statements”). This can overcount implemented stubs as placeholders.Strict docstring-only fix
def _is_placeholder_body(func_node: ast.FunctionDef) -> bool: @@ - # A docstring-only body has exactly one statement: an Expr containing a Constant string - if len(func_node.body) == 1: - stmt = func_node.body[0] - return isinstance(stmt, ast.Expr) and isinstance(stmt.value, ast.Constant) and isinstance(stmt.value.value, str) - # Also allow docstring + pass (common pattern) - if len(func_node.body) == 2: - first = func_node.body[0] - second = func_node.body[1] - is_docstring = ( - isinstance(first, ast.Expr) and isinstance(first.value, ast.Constant) and isinstance(first.value.value, str) - ) - is_pass = isinstance(second, ast.Pass) - return is_docstring and is_pass - return False + if len(func_node.body) != 1: + return False + statement = func_node.body[0] + return ( + isinstance(statement, ast.Expr) + and isinstance(statement.value, ast.Constant) + and isinstance(statement.value.value, str) + )Based on learnings,
__test__ = Falseis allowed only for STD placeholder tests with ONLY docstrings and no implementation code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/std_placeholder_stats/std_placeholder_stats.py` around lines 147 - 155, The placeholder detection currently treats a two-statement body with a docstring followed by a Pass as a placeholder (the block that checks len(func_node.body) == 2 and uses is_docstring and is_pass); change this to require a docstring-only body (i.e., only accept placeholder when the function/class body has a single Expr constant string and no other statements), remove the docstring+pass acceptance, and ensure any logic that allows `__test__ = False` is only applied when the body is strictly docstring-only rather than docstring+pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.pre-commit-config.yaml:
- Line 49: The flake8 exclude pattern under the exclude key currently removes
entire directories via the pattern
"(utilities/unittests/|utilities/junit_ai_utils\.py|scripts/std_placeholder_stats/tests/)"
— narrow this to file-scoped excludes instead: remove the directory-wide
fragments ("utilities/unittests/" and "scripts/std_placeholder_stats/tests/")
and replace them with explicit file patterns for the few known exceptions (e.g.
the specific test files in utilities/unittests that need E402 and the exact
files under scripts/std_placeholder_stats/tests/), keeping
utilities/junit_ai_utils\.py as-is; update the exclude string to list only those
exact filenames so unrelated/new files are still linted.
In `@scripts/std_placeholder_stats/std_placeholder_stats.py`:
- Around line 147-155: The placeholder detection currently treats a
two-statement body with a docstring followed by a Pass as a placeholder (the
block that checks len(func_node.body) == 2 and uses is_docstring and is_pass);
change this to require a docstring-only body (i.e., only accept placeholder when
the function/class body has a single Expr constant string and no other
statements), remove the docstring+pass acceptance, and ensure any logic that
allows `__test__ = False` is only applied when the body is strictly
docstring-only rather than docstring+pass.
In `@scripts/std_placeholder_stats/tests/test_std_placeholder_stats.py`:
- Around line 507-573: The test_handles_unreadable_files_gracefully test is
non-deterministic because it uses unreadable.chmod(0o000) to provoke an OSError
and doesn't exercise UnicodeDecodeError; instead, replace the chmod approach by
mocking Path.read_text (or scan_placeholder_tests' internal file read call) to
raise OSError for the unreadable path and create a second fixture file that
contains bytes not decodable as UTF-8 to trigger UnicodeDecodeError when read;
update assertions to ensure scan_placeholder_tests ignores both failing files
and still returns the readable file (reference
test_handles_unreadable_files_gracefully, scan_placeholder_tests, and
Path.read_text to locate where to apply the mock and where to create the
undecodable bytes file), and remove any chmod-based permission manipulation so
cleanup is deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e526e8d1-976b-4414-8c72-ea066147ccce
📒 Files selected for processing (3)
.pre-commit-config.yamlscripts/std_placeholder_stats/std_placeholder_stats.pyscripts/std_placeholder_stats/tests/test_std_placeholder_stats.py
openshift-virtualization-qe-bot
added
lgtm-coderabbitai[bot]
tox:verify-tc-requirement-polarion:running
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A CLI tool that scans the
tests/directory for STD (Standard Test Design) placeholder tests — tests marked with__test__ = Falsethat contain only docstrings describing expected behavior, without actual implementation.What it does
test_*.pyfiles using AST parsing__test__ = Falseat three levels:method.__test__ = FalseassignmentsUsage
Example output
Co-authored-by: Claude noreply@anthropic.com
Test plan
Summary by CodeRabbit
New Features
Tests
Chores