bazel: convert man_tcl_check to unittest py_test#9831
bazel: convert man_tcl_check to unittest py_test#9831oharboe wants to merge 11 commits intoThe-OpenROAD-Project:masterfrom
Conversation
Previously, man_tcl_check tests passed even when help/proc/readme counts didn't match because .ok golden files encoded the broken state. Add sys.exit(1) so the Python script exits non-zero on mismatch, and add set -o pipefail to regression_test.sh so the exit code propagates through the tee pipe. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Refactor docs/src/scripts/man_tcl_check.py from a standalone script with os.chdir() into an importable count_commands() function that takes explicit paths. Add BUILD with py_library so it can be used as a Bazel dependency. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Replace golden-file doc_check tests with proper unittest assertions. Each module's *_man_tcl_check.py is now a thin unittest.TestCase that calls count_commands() and asserts help == proc == readme counts. - Add doc_files filegroup to each module BUILD for data deps - Add py_test entries to each test BUILD - Remove from MANUAL_TESTS (no longer uses regression_test runner) - Delete .ok golden files (assertions replace output diffing) 9 modules pass (counts match), 15 fail with clear assertEqual messages showing the exact count mismatch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request refactors the man_tcl_check.py script into a modular count_commands function and converts the associated tests to unittest.TestCase using py_test rules. This significantly improves the test infrastructure by replacing golden file comparisons with explicit assertions, leading to clearer failure messages and better maintainability. The changes are well-structured and align with best practices for Python testing and modular design. A potential runtime error was introduced in the refactored script regarding the handling of missing README.md files, which has been highlighted for correction.
Guard against FileNotFoundError when a module has no README.md by checking os.path.exists() before opening. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Rename py_test targets from e.g. ant_man_tcl_check to ant_man_tcl_check_test to follow Bazel naming conventions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
/gemini review |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
Code Review
The pull request refactors the man_tcl_check.py script into a reusable count_commands function and integrates it into the Bazel build system. This involves creating filegroup targets in each module's BUILD file to expose documentation and TCL files, and then creating dedicated py_test targets for each module. These new tests use the refactored count_commands function to verify that the number of help, proc, and readme commands match within each module, replacing the previous ad-hoc script execution. Additionally, set -o pipefail is added to regression_test.sh for improved error handling. Review comments suggest improving the man_tcl_check.py script by replacing magic strings like "ICeWall", "PdnGen", and "pad", and the magic number 1 with named constants for better readability and maintainability.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
…_tcl_check Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Many modules have pre-existing mismatches between help, proc, and readme command counts. Assert against the actual expected counts per module so tests pass while still catching regressions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
src/ant/test/ant_man_tcl_check.py
Outdated
| test_dir = os.path.dirname(os.path.abspath(__file__)) | ||
| module_dir = os.path.dirname(test_dir) | ||
| h, p, r = count_commands(module_dir) | ||
| self.assertEqual(h, 0, f"help count ({h}) != expected (0)") |
There was a problem hiding this comment.
we should probably make this a constant on top of each file for easy maintenance e.g.
HELP_COUNT=0
PROC_COUNT=0
README_COUNT=1
...
self.assertEqual(p, PROC_COUNT, f"proc count ({p}) != expected ({PROC_COUNT})")
src/drt/test/drt_man_tcl_check.py
Outdated
| test_dir = os.path.dirname(os.path.abspath(__file__)) | ||
| module_dir = os.path.dirname(test_dir) | ||
| h, p, r = count_commands(module_dir) | ||
| self.assertEqual(h, p, f"help count ({h}) != proc count ({p})") |
There was a problem hiding this comment.
ideally, all should be assertEqual
|
clang-tidy review says "All clean, LGTM! 👍" |
… tests Address review feedback: extract expected counts into HELP_COUNT/PROC_COUNT/ README_COUNT constants at module level, use assertEqual with explicit expected values for all 24 modules (replacing relative h==p comparisons), and format with black. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
oharboe
force-pushed
the
man-tcl-check-unittest
branch
from
f212906 to
0379bc5
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
2 participants
Summary
docs/src/scripts/man_tcl_check.pyfrom a standalone script withos.chdir()into an importablecount_commands()function*_man_tcl_check.pytests tounittest.TestCasewithassertEqualassertionspy_testfrom@rules_pythoninstead of the shell-basedregression_testrunner.okgolden files (assertions replace output diffing)Depends on #9830.
What changed per module
src/<module>/BUILD: adddoc_filesfilegroup exportingREADME.md+src/*.tclsrc/<module>/test/BUILD: addpy_testentry, remove fromMANUAL_TESTSsrc/<module>/test/<module>_man_tcl_check.py: thin unittest wrapper callingcount_commands()src/<module>/test/<module>_man_tcl_check.ok: deletedWhy
The old golden-file approach tested output stability, not correctness —
.okfiles with "Command counts do not match." passed the test. The newassertEqualassertions fail immediately with a clear message likeAssertionError: 0 != 1 : proc count (0) != readme count (1).Test plan
🤖 Generated with Claude Code