refactor(tests): storage, serializer, and cloud based unit tests to follow test conventions#3249
Open
agsaru wants to merge 2 commits into
Open
refactor(tests): storage, serializer, and cloud based unit tests to follow test conventions#3249agsaru wants to merge 2 commits into
agsaru wants to merge 2 commits into
Conversation
…ollow test conventions
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR refactors and expands the unit test suite to be more parameterized, readable, and resilient to global registry state leaks (notably around serializer registration / bootstrap flows).
Changes:
- Converted many repetitive tests to
pytest.mark.parametrizewith clearer ids and docstrings. - Added/standardized fixtures to isolate or restore global
SerializerStorestate and to dynamically create serializer modules for lifecycle tests. - Improved coverage for serializer lifecycle behaviors (bootstrap, pending imports, retries, reset) and tightened S3 / CAS / Kubernetes / Argo CLI test structure.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/test_to_pod.py | Parameterized POD conversion tests and clarified callable/lambda expectations. |
| test/unit/test_serializer_public_api.py | Consolidated public API “not exported” checks and improved pickle-serializer smoke test assertions. |
| test/unit/test_serializer_lifecycle.py | Added fixtures for serializer store isolation and dynamic module creation; expanded lifecycle coverage. |
| test/unit/test_serializer_integration.py | Refactored integration tests, added store-restoration fixture, and clarified exception-flow scenarios. |
| test/unit/test_s3_storage.py | Introduced an S3Storage fixture and improved test readability around metadata preservation. |
| test/unit/test_s3_empty_input.py | Reworked empty-input regression tests into function-style tests with fixtures and parametrization. |
| test/unit/test_pickle_serializer.py | Improved isolation for ordering test and added clearer parametrization ids/docstrings. |
| test/unit/test_local_metadata_provider.py | Parameterized LocalMetadataProvider run-id deduction cases. |
| test/unit/test_kubernetes.py | Added ids and renamed tests for clearer intent around labels/key-value parsing. |
| test/unit/test_content_addressed_store.py | Modernized helpers/docs and strengthened assertions around error-message correctness. |
| test/unit/test_compute_resource_attributes.py | Consolidated resource-attribute merging tests into a single parametrized test matrix. |
| test/unit/test_aws_util.py | Simplified exception testing using nullcontext() vs manual try/except. |
| test/unit/test_artifact_serializer.py | Added registry restoration/cleanup fixtures and expanded serializer store + lazy import tests. |
| test/unit/test_argo_workflows_cli.py | Reorganized Argo CLI tests with ids/parametrization and consolidated trigger-explanation coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+23
to
+54
| @pytest.fixture | ||
| def isolated_store(): | ||
| """ | ||
| Fixture to isolate SerializerStore global state per test. | ||
| Provides a clean slate and restores original state afterward. | ||
| """ | ||
| # Snapshot original state | ||
| saved_all = dict(SerializerStore._all_serializers) | ||
| saved_active = set(SerializerStore._active_serializers) | ||
| saved_records = dict(SerializerStore._records) | ||
| saved_pending = dict(SerializerStore._pending_by_module) | ||
| saved_cache = SerializerStore._ordered_cache | ||
|
|
||
| # Clear state for test isolation | ||
| SerializerStore._all_serializers.clear() | ||
| SerializerStore._active_serializers.clear() | ||
| SerializerStore._records.clear() | ||
| SerializerStore._pending_by_module.clear() | ||
| SerializerStore._ordered_cache = None | ||
|
|
||
| yield | ||
|
|
||
| # Restore original state | ||
| SerializerStore._all_serializers.clear() | ||
| SerializerStore._all_serializers.update(saved_all) | ||
| SerializerStore._active_serializers.clear() | ||
| SerializerStore._active_serializers.update(saved_active) | ||
| SerializerStore._records.clear() | ||
| SerializerStore._records.update(saved_records) | ||
| SerializerStore._pending_by_module.clear() | ||
| SerializerStore._pending_by_module.update(saved_pending) | ||
| SerializerStore._ordered_cache = saved_cache |
Comment on lines
+23
to
+24
| @pytest.fixture | ||
| def isolated_store(): |
Comment on lines
+57
to
+58
| @pytest.fixture | ||
| def make_serializer_module(monkeypatch): |
Contributor
Greptile SummaryThis PR refactors 15 unit test files to follow project conventions: replacing
Confidence Score: 5/5This PR only touches test files; no production code is modified. The refactoring is structurally sound and behaviour-preserving across all 15 files. All changes are confined to the test layer. The fixture-based isolation correctly saves and restores registry state, the Docker-gating autouse fixture reproduces the original class-level skip logic faithfully, and the duplicate-test-name bug in test_kubernetes.py is silently fixed. The few open items from prior review threads are pre-existing concerns carried into this revision. test/unit/test_serializer_lifecycle.py — the inline interceptor cleanup in test_retry_fires_via_real_import_hook and the sys.modules leak for fixture_retry_dep are the only residual concerns, as noted in prior review threads. Important Files Changed
Reviews (2): Last reviewed commit: "added localbatch tests" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+439
to
+447
| assert rec.state == SerializerState.ACTIVE | ||
| assert rec.import_trigger == "hook" | ||
| assert ser_mod._E2E in SerializerStore._active_serializers | ||
|
|
||
| # Cleanup interceptor state to prevent leaking to other tests | ||
| from metaflow.datastore.artifacts.lazy_registry import _interceptor | ||
|
|
||
| _interceptor._watched.discard("fixture_retry_dep") | ||
| _interceptor._processed.discard("fixture_retry_dep") |
Contributor
There was a problem hiding this comment.
The _interceptor._watched and _interceptor._processed cleanup at lines 443–447 only runs when all three preceding assertions (lines 439–441) pass. If any of those assertions fail mid-test, "fixture_retry_dep" stays in _interceptor._watched, which persists for the entire process lifetime and can cause subsequent hook-related tests to behave unexpectedly.
The original code guarded this in a finally block. The isolated_store fixture restores the serializer registry but does not touch the interceptor's watched/processed sets, so there's no safety net here.
Comment on lines
+437
to
+447
| import fixture_retry_dep # noqa: F401 | ||
|
|
||
| assert rec.state == SerializerState.ACTIVE | ||
| assert rec.import_trigger == "hook" | ||
| assert ser_mod._E2E in SerializerStore._active_serializers | ||
|
|
||
| # Cleanup interceptor state to prevent leaking to other tests | ||
| from metaflow.datastore.artifacts.lazy_registry import _interceptor | ||
|
|
||
| _interceptor._watched.discard("fixture_retry_dep") | ||
| _interceptor._processed.discard("fixture_retry_dep") |
Contributor
There was a problem hiding this comment.
fixture_retry_dep not removed from sys.modules after the test
monkeypatch.delitem(sys.modules, "fixture_retry_dep", raising=False) at line 428 removes a pre-existing entry before the test. But the bare import fixture_retry_dep at line 437 inserts a new entry that monkeypatch knows nothing about and will not undo. The original finally block explicitly called sys.modules.pop("fixture_retry_dep", None) to handle exactly this. Leaving this module cached may affect other tests that depend on fixture_retry_dep being absent from sys.modules.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3249 +/- ##
=========================================
Coverage ? 28.80%
=========================================
Files ? 381
Lines ? 52467
Branches ? 9260
=========================================
Hits ? 15114
Misses ? 36344
Partials ? 1009 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Comment on lines
+42
to
+64
| @pytest.fixture | ||
| def isolated_store(): | ||
| """ | ||
| Fixture to isolate SerializerStore global state per test. | ||
| Prevents tests from poisoning the registry if they fail mid-execution. | ||
| """ | ||
| saved_all = dict(SerializerStore._all_serializers) | ||
| saved_active = set(SerializerStore._active_serializers) | ||
| saved_records = dict(SerializerStore._records) | ||
| saved_pending = dict(SerializerStore._pending_by_module) | ||
| saved_cache = SerializerStore._ordered_cache | ||
|
|
||
| yield | ||
|
|
||
| SerializerStore._all_serializers.clear() | ||
| SerializerStore._all_serializers.update(saved_all) | ||
| SerializerStore._active_serializers.clear() | ||
| SerializerStore._active_serializers.update(saved_active) | ||
| SerializerStore._records.clear() | ||
| SerializerStore._records.update(saved_records) | ||
| SerializerStore._pending_by_module.clear() | ||
| SerializerStore._pending_by_module.update(saved_pending) | ||
| SerializerStore._ordered_cache = saved_cache |
Comment on lines
+284
to
+304
| port = 18766 | ||
| store = Store(queue_name="inject-queue") | ||
| runner = DockerRunner( | ||
| store, | ||
| host_addr="host.docker.internal", | ||
| port=port, | ||
| inject_env={"LOCALBATCH_CANARY": "canary-value"}, | ||
| ) | ||
| app = create_app(store, runner) | ||
| server = uvicorn.Server( | ||
| uvicorn.Config(app, host="127.0.0.1", port=port, log_level="warning") | ||
| ) | ||
| t = threading.Thread(target=server.run, daemon=True) | ||
| t.start() | ||
|
|
||
| for _ in range(100): | ||
| try: | ||
| requests.get(f"http://127.0.0.1:{port}/health", timeout=0.1) | ||
| break | ||
| except Exception: | ||
| time.sleep(0.05) |
Comment on lines
+340
to
+341
| server.should_exit = True | ||
| t.join(timeout=5) |
Comment on lines
+23
to
+41
| @pytest.fixture | ||
| def isolated_store(): | ||
| """ | ||
| Fixture to isolate SerializerStore global state per test. | ||
| Provides a clean slate and restores original state afterward. | ||
| """ | ||
| # Snapshot original state | ||
| saved_all = dict(SerializerStore._all_serializers) | ||
| saved_active = set(SerializerStore._active_serializers) | ||
| saved_records = dict(SerializerStore._records) | ||
| saved_pending = dict(SerializerStore._pending_by_module) | ||
| saved_cache = SerializerStore._ordered_cache | ||
|
|
||
| # Clear state for test isolation | ||
| SerializerStore._all_serializers.clear() | ||
| SerializerStore._active_serializers.clear() | ||
| SerializerStore._records.clear() | ||
| SerializerStore._pending_by_module.clear() | ||
| SerializerStore._ordered_cache = None |
agsaru
changed the title
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