diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 522ff5ac..0ae3f868 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -231,6 +231,17 @@ jobs: python-version: ['3.10', '3.11', '3.12', '3.13'] timeout-minutes: 15 + # Without this the matrix is decorative. `make dev` runs a bare `uv sync`, + # and uv resolves its interpreter from .python-version (pinned to 3.11) + # rather than from whatever actions/setup-python just installed -- so every + # leg built a 3.11 venv and all four ran the same interpreter. UV_PYTHON + # takes precedence over the pin file, which restores real version coverage. + # + # Set at job level, not on the install step: `make ci-quality-github` runs + # `uv run pytest`, which resolves the interpreter again. + env: + UV_PYTHON: ${{ matrix.python-version }} + steps: - name: Checkout code uses: actions/checkout@v4 @@ -252,6 +263,28 @@ jobs: - name: Quality checks run: make ci-quality-github + # Shared with the other repos' coverage jobs. It lives in its own public + # repo because a public repository -- this one -- cannot resolve an action + # from a private one. An earlier attempt to consume it from the private + # internal-workflows repo failed at `Set up job` on every matrix leg with + # "Unable to resolve action, not found", so no tests ran at all. + # + # Pinned by commit rather than tag: a tag can be repointed at new code, + # and pinning means a change to the action cannot reach us until we bump + # it deliberately. + # + # This owns the coverage artifact upload. The test-results upload below + # stays separate -- the action uploads one artifact, and these are two. + - name: Coverage summary + if: always() + uses: runpod/coverage-summary-action@65b35a32ecfd9c2f0dc2352394eca1decfba4915 + with: + format: cobertura + coverage-file: coverage.xml + results: pytest-results-*.xml + artifact-name: coverage-${{ matrix.python-version }} + if-no-files-found: warn + - name: Upload test results uses: actions/upload-artifact@v4 if: always() diff --git a/Makefile b/Makefile index c764d4d4..46b253b3 100644 --- a/Makefile +++ b/Makefile @@ -72,8 +72,10 @@ test-integration-serial: # Run integration tests serially (for debugging) uv run pytest tests/integration/ -v -m integration test-coverage: # Run tests with coverage report (parallel non-serial, then serial pass for state isolation) - uv run pytest tests/ -v -n auto -m "not serial" --cov=runpod_flash --cov-report=xml - uv run pytest tests/ -v -m "serial" --cov=runpod_flash --cov-append --cov-report=term-missing + uv run pytest tests/ -v -n auto -m "not serial" --cov=runpod_flash --cov-branch --cov-report=xml + # Re-emit the XML after appending the serial tests, or coverage.xml is left + # holding only the parallel run and undercounts. + uv run pytest tests/ -v -m "serial" --cov=runpod_flash --cov-branch --cov-append --cov-report=term-missing --cov-report=xml test-coverage-serial: # Run tests with coverage report (serial execution) uv run pytest tests/ -v --cov=runpod_flash --cov-report=term-missing @@ -124,10 +126,24 @@ ci-quality-github: # Quality checks with GitHub Actions formatting (parallel by uv run ruff check . --output-format=github @echo "::endgroup::" @echo "::group::Test suite with coverage (parallel non-serial)" - uv run pytest tests/ --junitxml=pytest-results-parallel.xml -v -n auto -m "not serial" --cov=runpod_flash --cov-report=xml --cov-fail-under=0 + uv run pytest tests/ --junitxml=pytest-results-parallel.xml -v -n auto -m "not serial" --cov=runpod_flash --cov-branch --cov-report=xml --cov-fail-under=0 @echo "::endgroup::" @echo "::group::Test suite with coverage (serial pass)" - uv run pytest tests/ --junitxml=pytest-results-serial.xml -v -m "serial" --cov=runpod_flash --cov-append --cov-report=term-missing + # Re-emit the XML after appending the serial tests, or coverage.xml is left + # holding only the parallel run and undercounts. + # + # This line, not the parallel one above, is the real coverage gate: the + # parallel invocation passes --cov-fail-under=0 to suppress the partial + # number, while this one inherits --cov-fail-under=65 from pyproject + # addopts. Do not "align" the two flags — that quietly disables the gate. + # + # Note also that make stops at the first failing recipe line, so if the + # parallel pass fails this re-emit never runs and coverage.xml holds the + # parallel subset. That is deliberate: guarding the parallel line with + # `-`/`|| true` so this always ran would let a red parallel suite exit 0. + # The weekly report only reads runs whose status is success, so an + # undercount on an already-failing run is not consumed by anything. + uv run pytest tests/ --junitxml=pytest-results-serial.xml -v -m "serial" --cov=runpod_flash --cov-branch --cov-append --cov-report=term-missing --cov-report=xml @echo "::endgroup::" ci-quality-github-serial: # Serial quality checks for GitHub Actions (for debugging) diff --git a/tests/unit/cli/commands/test_run_server_helpers.py b/tests/unit/cli/commands/test_run_server_helpers.py index 60ae9a14..80a3feb6 100644 --- a/tests/unit/cli/commands/test_run_server_helpers.py +++ b/tests/unit/cli/commands/test_run_server_helpers.py @@ -336,12 +336,34 @@ async def test_allows_populated_fields(self): assert result == {"echo": "hello"} func.assert_called_once_with(msg="hello") + # These two wrap the mock in a real async def instead of passing the mock + # straight to call_with_body. + # + # call_with_body -> _map_body_to_params calls inspect.signature(func), and + # signature() of a Mock is version-dependent: on 3.10 it raises + # `TypeError: 'Mock' object is not subscriptable` (Mock auto-creates a + # __signature__ child, which inspect then tries to use), while on 3.11+ it + # reports (*args, **kwargs). Passing spec= does not help -- the failure is + # in signature(), not in spec resolution. + # + # call_with_body catches Exception and converts it into a 500 JSONResponse, + # so on 3.10 these failed as `assert == {'ok': True}`, + # with the real TypeError visible only inside the response body. + # + # The wrapper declares (*args, **kwargs), the same signature 3.11+ inferred + # from the Mock, so both branches of _map_body_to_params behave exactly as + # before -- and delegating to the mock keeps the call assertions. + @pytest.mark.asyncio async def test_allows_plain_dict_body(self): """Non-empty plain dict passes through unchanged (no model_fields_set).""" body = {"key": "value"} - func = AsyncMock(return_value={"ok": True}) + mock = AsyncMock(return_value={"ok": True}) + + async def func(*args, **kwargs): + return await mock(*args, **kwargs) + result = await call_with_body(func, body) assert result == {"ok": True} @@ -354,8 +376,12 @@ async def test_allows_empty_plain_dict_body(self): """ body = {} - func = AsyncMock(return_value={"ok": True}) + mock = AsyncMock(return_value={"ok": True}) + + async def func(*args, **kwargs): + return await mock(*args, **kwargs) + result = await call_with_body(func, body) assert result == {"ok": True} - func.assert_called_once_with() + mock.assert_called_once_with() diff --git a/tests/unit/core/resources/test_resource_manager_extended.py b/tests/unit/core/resources/test_resource_manager_extended.py index 06f2ac1e..fe24a8f8 100644 --- a/tests/unit/core/resources/test_resource_manager_extended.py +++ b/tests/unit/core/resources/test_resource_manager_extended.py @@ -9,6 +9,17 @@ ) +class _LegacyResource: + """Minimal picklable stand-in for a resource in the pre-1.12 state file. + + Module-level, not defined inside a test: cloudpickle serialises a class + defined in a function body by value, which is the thing being avoided here. + """ + + def __init__(self, config_hash: str): + self.config_hash = config_hash + + @pytest.fixture(autouse=True) def reset_manager(isolate_resource_state_file, reset_singletons): """Reset singleton and state file between tests.""" @@ -56,7 +67,13 @@ def test_loads_legacy_dict_format(self, tmp_path): state_file = tmp_path / ".runpod" / "resources.pkl" state_file.parent.mkdir(parents=True) - resources = {"key1": MagicMock(config_hash="hash1")} + # A plain object rather than MagicMock: whether cloudpickle can pickle a + # Mock is version-dependent, and on 3.10 this failed with + # "Could not pickle object as excessively deep recursion required". + # Only .config_hash is read here (via _refresh_config_hashes), and the + # absence of get_resource_key is what keeps the legacy key un-migrated, + # which is exactly what this test is asserting. + resources = {"key1": _LegacyResource("hash1")} with open(state_file, "wb") as f: cloudpickle.dump(resources, f) diff --git a/tests/unit/test_deployment.py b/tests/unit/test_deployment.py index 081ca0b9..15c479c6 100644 --- a/tests/unit/test_deployment.py +++ b/tests/unit/test_deployment.py @@ -195,19 +195,44 @@ async def slow_deploy(resource): assert results[0].duration >= 0.09 def test_deploy_all_background(self, mock_resources): - """Test background deployment doesn't block.""" + """Test background deployment starts a daemon thread without blocking.""" orchestrator = DeploymentOrchestrator() - with patch.object( - orchestrator.manager, "get_or_deploy_resource", new_callable=AsyncMock - ) as mock_deploy: + # threading.Thread is stubbed rather than left to run for real. + # + # deploy_all_background() starts a daemon thread and returns + # immediately, so a real thread outlives this test: the patch below + # would be lifted while the thread was still starting, and the thread + # would then call the *unpatched* manager and register these + # MagicMock(spec=ServerlessResource) objects into the ResourceManager + # singleton -- during whichever unrelated test happened to be running + # at that moment. ResourceManager._save_resources() cloudpickles its + # whole state on every registration, and a MagicMock cannot be pickled, + # so an arbitrary later test died with + # + # _pickle.PicklingError: args[0] from __newobj__ args has the wrong class + # + # Which test got hit depended on thread scheduling and on xdist's + # worker assignment, which is what made it flaky rather than simply + # broken. Stubbing the thread keeps the assertion this test actually + # makes -- that the call is non-blocking and spawns a daemon thread -- + # and lets nothing escape the test. + with ( + patch("runpod_flash.core.deployment.threading.Thread") as mock_thread_cls, + patch.object( + orchestrator.manager, "get_or_deploy_resource", new_callable=AsyncMock + ) as mock_deploy, + ): mock_deploy.side_effect = mock_resources # Should not block orchestrator.deploy_all_background(mock_resources) - # Background thread should be started - # (not much we can test here without waiting for thread) + # A daemon thread was started, and nothing ran inline. + mock_thread_cls.assert_called_once() + assert mock_thread_cls.call_args.kwargs["daemon"] is True + mock_thread_cls.return_value.start.assert_called_once() + mock_deploy.assert_not_called() def test_deploy_all_background_empty_list(self): """Test background deployment with empty list.""" diff --git a/uv.lock b/uv.lock index a2e48d3f..4db98f5a 100644 --- a/uv.lock +++ b/uv.lock @@ -1,6 +1,6 @@ version = 1 revision = 3 -requires-python = ">=3.10, <3.13" +requires-python = ">=3.10, <3.14" resolution-markers = [ "python_full_version >= '3.13'", "python_full_version < '3.13'", @@ -824,7 +824,7 @@ name = "exceptiongroup" version = "1.3.1" source = { registry = "https://pypi.org/simple" } dependencies = [ - { name = "typing-extensions" }, + { name = "typing-extensions", marker = "python_full_version < '3.13'" }, ] sdist = { url = "https://files.pythonhosted.org/packages/50/79/66800aadf48771f6b62f7eb014e352e5d06856655206165d775e675a02c9/exceptiongroup-1.3.1.tar.gz", hash = "sha256:8b412432c6055b0b7d14c310000ae93352ed6754f70fa8f7c34141f91c4e3219", size = 30371, upload-time = "2025-11-21T23:01:54.787Z" } wheels = [ @@ -2669,6 +2669,7 @@ dependencies = [ { name = "rich" }, { name = "runpod" }, { name = "tomli", marker = "python_full_version < '3.11'" }, + { name = "tomlkit" }, { name = "typer" }, ] @@ -2698,6 +2699,7 @@ requires-dist = [ { name = "rich", specifier = ">=14.0.0" }, { name = "runpod", git = "https://github.com/runpod/runpod-python?rev=main" }, { name = "tomli", marker = "python_full_version < '3.11'", specifier = ">=2.0.0" }, + { name = "tomlkit", specifier = ">=0.13.0" }, { name = "typer", specifier = ">=0.12.0" }, ]