refactor(tests): enforce single-pass testing and remove duplicate tests#249
Conversation
Refactor 5 pass test files to follow project conventions: - Use @pl.program Before/Expected pattern instead of IR builder - Each test tests ONE pass only (no chaining run_verifier/init_mem_ref) - Remove 15 duplicate/redundant tests across files - Remove integration tests using PassManager - Add _prepare_and_run_* helpers to clarify test setup vs pass under test
📝 WalkthroughWalkthroughTest infrastructure refactored to use Python-based PL dialect (pypto.language) instead of IR builder patterns. New test helpers introduced for sequential pass execution. Some test coverage removed (Type Preservation, Pass Pipeline categories). Skill file updated for automatic branch name generation based on change type. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
Summary of ChangesHello @Hzfengsy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the testing infrastructure for IR transforms, aiming to improve the clarity, maintainability, and efficiency of the test suite. By enforcing single-pass testing, removing redundant tests, and introducing dedicated helper functions for test setup, the changes make it easier to understand and verify the behavior of individual optimization passes. This streamlines the development workflow and reduces the cognitive load for future test modifications. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.claude/skills/github-pr/SKILL.md (1)
47-52: Consider adding brief decision criteria for prefix selection.While "based on the changes" is likely sufficient for an AI assistant to determine the appropriate prefix, adding a brief note about decision criteria could improve consistency and clarity. For example:
feat/— new functionality or capabilitiesfix/— bug fixes or correctionsrefactor/— code restructuring without behavior changetest/— test-only changesdocs/— documentation-only changeschore/— maintenance, dependencies, toolingThis would help ensure consistent branch naming across different scenarios, especially when changes span multiple categories.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/github-pr/SKILL.md around lines 47 - 52, Add a short decision-criteria note immediately after the auto-generation prefix list that explains when to pick each prefix (e.g., feat/ for new functionality, fix/ for bug fixes, refactor/ for behavior-preserving restructures, test/ for test changes, docs/ for documentation-only edits, chore/ for maintenance/deps) so the auto-generated branch name rule ("Auto-generate a branch name with a meaningful prefix...") has explicit, consistent selection guidance; update the surrounding prose that references the prefixes and keep the existing git checkout example unchanged.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.claude/skills/github-pr/SKILL.mdtests/ut/ir/transforms/test_add_alloc_pass.pytests/ut/ir/transforms/test_basic_memory_reuse.pytests/ut/ir/transforms/test_convert_to_ssa_pass.pytests/ut/ir/transforms/test_flatten_call_expr_pass.pytests/ut/ir/transforms/test_insert_sync.py
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.claude/skills/github-pr/SKILL.md:
- Around line 47-52: Add a short decision-criteria note immediately after the
auto-generation prefix list that explains when to pick each prefix (e.g., feat/
for new functionality, fix/ for bug fixes, refactor/ for behavior-preserving
restructures, test/ for test changes, docs/ for documentation-only edits, chore/
for maintenance/deps) so the auto-generated branch name rule ("Auto-generate a
branch name with a meaningful prefix...") has explicit, consistent selection
guidance; update the surrounding prose that references the prefixes and keep the
existing git checkout example unchanged.
![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 test files to enforce single-pass testing and removes duplicate tests, improving test clarity and efficiency. The changes involve converting IR builder to @pl.program DSL, removing PassManager integration tests, and adding helper functions for test setup. The .claude/skills/github-pr/SKILL.md file was also updated to auto-generate branch names. Overall, the changes align with the goal of streamlining the testing infrastructure.
I am having trouble creating individual review comments. Click here to see my feedback.
.claude/skills/github-pr/SKILL.md (47)
The instruction to auto-generate a branch name is a good improvement for workflow efficiency. This aligns with the repository's focus on automation and clear task definitions, as outlined in the .claude/skills/github-pr/SKILL.md document itself.
References
- All AI instructions for the PyPTO project are centralized in the
.claude/directory. For code review tasks, consult.claude/skills/code-review/SKILL.mdfor comprehensive review guidelines including code quality standards, documentation alignment, cross-layer consistency, and common issues to check. (link)
tests/ut/ir/transforms/test_add_alloc_pass.py (10-12)
The removal of DataType, ir.builder, ir.op.block, OptimizationStrategy, and PassManager imports is appropriate given the refactoring to use @pl.program DSL and single-pass testing. This reduces unnecessary dependencies and cleans up the import statements.
tests/ut/ir/transforms/test_add_alloc_pass.py (106-114)
The new helper function _prepare_and_run_add_alloc clearly separates the test setup (init_mem_ref) from the pass under test (add_alloc). This improves the readability and maintainability of the tests, making it easier to understand what each test is specifically validating.
tests/ut/ir/transforms/test_add_alloc_pass.py (127-138)
Converting the IR builder pattern to the @pl.program DSL significantly improves the readability and conciseness of the test case setup. This change aligns with the overall goal of using the DSL for defining IR programs in tests.
tests/ut/ir/transforms/test_add_alloc_pass.py (140)
Using the _prepare_and_run_add_alloc helper function here simplifies the test logic by encapsulating the common setup and pass execution steps.
tests/ut/ir/transforms/test_add_alloc_pass.py (189-201)
The adoption of @pl.program DSL for defining the Before program makes the test setup much cleaner and more declarative, which is a significant improvement over the manual IRBuilder approach.
tests/ut/ir/transforms/test_add_alloc_pass.py (203)
Consolidating the pass execution into _prepare_and_run_add_alloc reduces duplication and makes the test flow more explicit.
tests/ut/ir/transforms/test_add_alloc_pass.py (243-247)
The use of @pl.program for the Before class simplifies the definition of the empty function, making the test case more readable.
tests/ut/ir/transforms/test_add_alloc_pass.py (249)
Directly applying passes.add_alloc() to Before is correct for testing a single pass without additional setup, as add_alloc does not depend on init_mem_ref for functions without TileType variables.
tests/ut/ir/transforms/test_add_alloc_pass.py (261)
The removal of the @pytest.mark.xfail decorator is appropriate. The original xfail reason indicated a dependency on InitMemRefPass which is now handled by the _prepare_and_run_add_alloc helper, ensuring the test runs as expected.
tests/ut/ir/transforms/test_add_alloc_pass.py (270-281)
The use of @pl.program for defining the Before program here streamlines the test setup, making it more declarative and easier to read.
tests/ut/ir/transforms/test_add_alloc_pass.py (283)
Utilizing _prepare_and_run_add_alloc ensures that the necessary init_mem_ref pass is run before add_alloc, addressing the previous xfail condition and making the test self-contained.
tests/ut/ir/transforms/test_add_alloc_pass.py (330-341)
The conversion to @pl.program DSL for defining the Before program simplifies the test code and makes it more consistent with other tests using the DSL.
tests/ut/ir/transforms/test_add_alloc_pass.py (343)
Using the _prepare_and_run_add_alloc helper function here centralizes the setup and execution of the passes, making the test more robust and easier to maintain.
tests/ut/ir/transforms/test_basic_memory_reuse.py (14)
The removal of OptimizationStrategy and PassManager imports is consistent with the PR's goal of removing PassManager integration tests and focusing on single-pass testing.
tests/ut/ir/transforms/test_basic_memory_reuse.py (292-318)
The removal of test_with_pass_manager is appropriate as it was an integration test using PassManager, which is being removed in favor of single-pass testing.
tests/ut/ir/transforms/test_convert_to_ssa_pass.py (167-179)
Adding a test case to ensure that already-SSA code remains unchanged after conversion is a good practice. This verifies the idempotency of the convert_to_ssa pass, which is important for correctness and efficiency.
tests/ut/ir/transforms/test_convert_to_ssa_pass.py (673-751)
The removal of TestTypePreservation and TestPassPipeline classes, along with their associated tests, is consistent with the PR's goal of removing duplicate tests and enforcing single-pass testing. The type preservation aspects are implicitly covered by other tests, and pass pipeline tests are being removed.
tests/ut/ir/transforms/test_convert_to_ssa_pass.py (895-941)
The removal of test_single_operation_no_reassignment and test_many_reassignments is justified as these patterns are covered by test_single_reassignment and test_multiple_reassignments in TestStraightLineCode, reducing redundancy.
tests/ut/ir/transforms/test_convert_to_ssa_pass.py (997-1021)
The removal of test_chain_of_reassignments is appropriate as its functionality is already covered by test_multiple_reassignments in TestStraightLineCode, ensuring test suite conciseness.
tests/ut/ir/transforms/test_convert_to_ssa_pass.py (1137-1161)
The removal of test_backward_compat_explicit_iter_args is acceptable. While backward compatibility is important, if the new DSL syntax is consistently enforced, this specific test might become redundant.
tests/ut/ir/transforms/test_convert_to_ssa_pass.py (978-990)
Adding the Expected program for test_nested_for_loops_multiple_vars_plain with assert_structural_equal is a significant improvement. This makes the test more robust by directly comparing the transformed IR structure, rather than just relying on run_verifier which only checks for validity.
tests/ut/ir/transforms/test_convert_to_ssa_pass.py (1010-1023)
Adding the Expected program for test_for_with_if_inside_plain with assert_structural_equal enhances the test's precision. This ensures the SSA conversion correctly handles conditional assignments within loops.
tests/ut/ir/transforms/test_convert_to_ssa_pass.py (1044-1059)
Adding the Expected program for test_nested_loops_with_if_plain with assert_structural_equal improves the test's reliability. This verifies that nested loops with conditional assignments are correctly converted to SSA form.
tests/ut/ir/transforms/test_convert_to_ssa_pass.py (1082-1099)
Adding the Expected program for test_complex_nested_control_flow_plain with assert_structural_equal significantly strengthens this test case. It ensures that the SSA conversion handles intricate nested control flow and multiple variable modifications correctly.
tests/ut/ir/transforms/test_convert_to_ssa_pass.py (1297-1312)
The removal of test_deeply_nested_loops_plain is acceptable. While deeply nested loops are a valid test case, if the SSA conversion logic for nested loops is generally robust, this specific test might be redundant.
tests/ut/ir/transforms/test_convert_to_ssa_pass.py (1151-1166)
Adding the Expected program for test_if_modifying_different_vars_plain with assert_structural_equal improves the test's precision. This ensures that the SSA conversion correctly handles conditional assignments to different variables.
tests/ut/ir/transforms/test_flatten_call_expr_pass.py (25-32)
The updated docstring for NormalizeIR clarifies its role as a test utility for structural comparison, explicitly stating that it's not a second pass under test. This is important for understanding the test's intent and preventing misinterpretation.
def NormalizeIR(program):
"""Normalize Expected IR structure to match flatten_call_expr pass output.
This is a test comparison utility, not a second pass under test.
The flatten_call_expr pass internally applies normalize_stmt_structure
before and flatten_single_stmt after call expression flattening. Expected
IR from the DSL must go through the same structural transformations for
assert_structural_equal to succeed.
"""
return passes.flatten_single_stmt()(passes.normalize_stmt_structure()(program))
tests/ut/ir/transforms/test_flatten_call_expr_pass.py (514-544)
The removal of TestFlattenWithVerifier is consistent with the PR's goal of removing duplicate tests and enforcing single-pass testing. The assert_structural_equal calls in other tests provide sufficient verification of the pass's output.
There was a problem hiding this comment.
Pull request overview
This PR refactors the test suite to enforce a single-pass testing principle, ensuring each test focuses on exactly one transformation pass. It removes 15 duplicate and integration tests (reducing the count from 351 to 340), converts tests from the old IR builder pattern to the modern @pl.program DSL, and replaces verification-only tests with proper Before/Expected structural equality checks.
Changes:
- Enforce single-pass testing by removing pass chaining (e.g.,
convert_to_ssafollowed byrun_verifier) - Remove 15 duplicate/integration tests while preserving test coverage
- Add
_prepare_and_run_*helper pattern to clarify test setup vs pass under test - Convert remaining IR builder tests to
@pl.programDSL - Add
Expectedprograms to tests that previously only used verification
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test_insert_sync.py | Remove unnecessary init_mem_ref() calls since tiles already have memrefs from construction |
| test_flatten_call_expr_pass.py | Remove duplicate TestFlattenWithVerifier class that tested both flattening and verification; clarify NormalizeIR as test utility |
| test_convert_to_ssa_pass.py | Remove 11 duplicate tests (TestTypePreservation, TestPassPipeline, duplicate edge cases); add Expected programs to 5 tests that only used verification |
| test_basic_memory_reuse.py | Rename helper to _prepare_and_run_memory_reuse with clear docstring; remove PassManager integration test |
| test_add_alloc_pass.py | Convert from IR builder to @pl.program DSL; add _prepare_and_run_add_alloc helper; remove 2 PassManager integration tests and @pytest.mark.xfail |
| .claude/skills/github-pr/SKILL.md | Auto-generate branch names instead of prompting user, improving AI assistant workflow |
1 participant
Summary
@pl.programrun_verifier/init_mem_ref)PassManager_prepare_and_run_*helpers to clarify test setup vs pass under testgithub-prskill to auto-generate branch namesFiles changed
test_convert_to_ssa_pass.py— remove 11 duplicates, addExpectedprograms withassert_structural_equal, removeTestTypePreservation/TestConvertToSSAVariousPatternsclassestest_add_alloc_pass.py— convert IR builder to@pl.programDSL, remove 2PassManagertests, remove@pytest.mark.xfailtest_basic_memory_reuse.py— rename helper, removePassManagertesttest_flatten_call_expr_pass.py— remove duplicateTestFlattenWithVerifierclasstest_insert_sync.py— remove unnecessaryinit_mem_ref()calls.claude/skills/github-pr/SKILL.md— auto-generate branch names instead of asking userTesting