refactor(tests): cmd tests to follow test conventions#3246
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors existing pytest suites under test/cmd to use function-based tests, tmp_path, and pytest-mock fixtures, improving consistency and reducing reliance on tempfile and unittest.mock.
Changes:
- Rewrote
metaflow diffcommand tests to usemockerandtmp_pathfixtures. - Refactored
StubGeneratortests into fixture-driven, function-based tests and expanded parametrization. - Updated mocking approach from
unittest.mockdecorators/context managers topytest-mock.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/cmd/diff/test_metaflow_diff.py | Refactors diff-related tests to mocker/tmp_path and updates expectations for extract_code_package/perform_diff. |
| test/cmd/develop/test_stub_generator.py | Converts class-based tests to fixtures + standalone tests and updates mocking to pytest-mock. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummaryThis PR refactors two test files from unittest class-based style to standalone pytest functions, replacing
Confidence Score: 5/5Pure test refactoring with no changes to production code; all assertions are equivalent or stricter than before. Every changed line is in test files. The refactoring faithfully preserves all prior assertions, the updated mock interface for code.extract() now matches the actual implementation, and pytest fixture scoping ensures correct teardown. No new test logic is omitted or regressed. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "replaced '==' with is" | 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 #3246 +/- ##
=========================================
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:
|
| def test_op_patch_writes_diff_output_to_file(mocker, tmp_path): | ||
| """Test that op_patch successfully writes the generated diff to the specified patch file.""" | ||
| mock_perform_diff = mocker.patch("metaflow.cmd.code.perform_diff") | ||
| mock_perform_diff.return_value = ["diff --git a/file.txt b/file.txt\n"] | ||
|
|
||
| patch_file = tmp_path / "patch.patch" | ||
|
|
||
| # Act | ||
| op_patch(str(tmp_path), str(patch_file)) | ||
|
|
||
| # Assert | ||
| mock_perform_diff.assert_called_once_with(str(tmp_path), output=True) | ||
| assert patch_file.read_text() == "diff --git a/file.txt b/file.txt\n" |
| @pytest.fixture | ||
| def mock_getmodule(mocker): | ||
| """Helper fixture to easily mock inspect.getmodule to return a specific module name.""" | ||
|
|
||
| with patch("inspect.getmodule", return_value=mock_module): | ||
| stub = self.generator._generate_function_stub("test_func", test_func) | ||
| def _mocker(module_name="test.module"): | ||
| mock_module = mocker.Mock() | ||
| mock_module.__name__ = module_name | ||
| return mocker.patch("inspect.getmodule", return_value=mock_module) | ||
|
|
||
| # Should not contain class objects | ||
| assert "<class '" not in stub | ||
| assert "def test_func(" in stub | ||
| assert "test.module.TestClass" in stub | ||
| return _mocker |
PR Type
Summary