refactor(tests): unit test submodules to follow test conventions#3248
Open
agsaru wants to merge 4 commits into
Open
refactor(tests): unit test submodules to follow test conventions#3248agsaru wants to merge 4 commits into
agsaru wants to merge 4 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors and reorganizes the Python unit/integration tests to be more consistent, readable, and maintainable—primarily by converting class-based tests into function tests, introducing shared fixtures, and consolidating repeated assertions via parametrization.
Changes:
- Convert multiple test modules from class-based tests to function-based tests, adding shared fixtures where helpful.
- Consolidate repeated assertions using
pytest.mark.parametrizefor graph inference, configs, and inheritance tests. - Improve localbatch docker skipping behavior by centralizing docker availability checks for
@pytest.mark.dockertests.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/spin/test_spin.py | Reorganizes spin tests with clearer sections/names and removes debug prints. |
| test/unit/mutators/test_string_step_mutator.py | Converts class-based tests to standalone functions with docstrings. |
| test/unit/mutators/test_remove_decorator_guard.py | Replaces helper with a fixture factory and flattens tests to functions. |
| test/unit/mutators/test_post_step_none_false.py | Converts class-based regression tests to explicit standalone tests. |
| test/unit/mutators/test_flow_mutator_addition.py | Converts class-based tests to functions; adds clearer separation/comments. |
| test/unit/mutators/test_dual_inheritance.py | Adds shared fixtures for task data; converts to function tests. |
| test/unit/mutators/test_add_decorator_returns.py | Adds shared fixture for task data and expands test descriptions. |
| test/unit/localbatch/test_localbatch.py | Flattens tests, adds an autouse docker-skip fixture, and improves organization. |
| test/unit/inheritance/test_inheritance.py | Reorganizes inheritance tests into themed sections and parametrized integration tests. |
| test/unit/graph_inference/test_graph_inference.py | Consolidates repeated “shape” assertions using parametrization and groups edge cases. |
| test/unit/graph_inference/test_card_dag.py | Introduces fixtures for graph inputs and clarifies helper docstring. |
| test/unit/configs/test_config_plain.py | Replaces repetitive assertions with parametrized checks plus focused property tests. |
| test/unit/configs/test_config_naming.py | Parametrizes underscore/dash/mixed naming assertions to reduce duplication. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+23
to
32
| def test_simple_flows_validate_artifacts(flow_file, fixture_name, request): | ||
| """Test that basic flows run steps correctly and validate their artifacts.""" | ||
| run = request.getfixturevalue(fixture_name) | ||
| print(f"Running test for {flow_file}: {run}") | ||
|
|
||
| # Act & Assert: Iterate through and run each step | ||
| for step in run.steps(): | ||
| print("-" * 100) | ||
| if fixture_name == "complex_dag_run": | ||
| run_step(flow_file, run, step.id, environment="conda") | ||
| else: | ||
| run_step(flow_file, run, step.id) |
Comment on lines
+193
to
+208
| def test_spin_generates_cards_correctly(simple_card_run): | ||
| """Test that spinning a flow with the @card decorator successfully outputs cards.""" | ||
| # Setup | ||
| from metaflow.cards import get_cards | ||
|
|
||
| step_name = "start" | ||
| task = simple_card_run[step_name].task | ||
| flow_path = os.path.join(FLOWS_DIR, "simple_card_flow.py") | ||
| print(f"Running test for cards in {flow_path}: {simple_card_run}") | ||
|
|
||
| # Act | ||
| with Runner(flow_path, cwd=FLOWS_DIR).spin(task.pathspec, persist=True) as spin: | ||
| spin_task = spin.task | ||
| from metaflow.cards import get_cards | ||
| res = get_cards(spin.task, follow_resumed=False) | ||
|
|
||
| res = get_cards(spin_task, follow_resumed=False) | ||
| print(res) | ||
| # Assert | ||
| assert res is not None, "Cards should be generated and retrievable" | ||
| # Optional: assert len(res) > 0 if you expect a specific number of cards |
Contributor
Greptile SummaryThis PR refactors 12 unit test submodules across
Confidence Score: 5/5Pure structural refactoring of test files with no changes to production code; all original assertions are preserved. Every original assertion is accounted for in the refactored code. The cards test gains stronger coverage with real assertions replacing bare print statements, and the broken assert_artifacts call from a previous review is resolved. No fixture scoping issues were found. No files require special attention. Important Files Changed
Reviews (4): Last reviewed commit: "revert test_simple_flows_validate_artifa..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3248 +/- ##
=========================================
Coverage ? 28.85%
=========================================
Files ? 381
Lines ? 52467
Branches ? 9260
=========================================
Hits ? 15138
Misses ? 36315
Partials ? 1014 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
PR Type
Summary