Repository navigation
Conversation
negz
changed the title
ytsarev
reviewed
Oct 2, 2026
| {"model": serving.model, "messages": [{"role": "user", "content": "ping"}]}, | ||
| ) | ||
| assert r.status == 200, r | ||
| assert r.json()["model"] == "ml-team/mock-demo" |
Contributor
There was a problem hiding this comment.
Much more powerful and flexible assertion capabilities for E2E test now. Looks great to me.
The checks build their virtualenvs from a uv2nix package set that checks.nix defines for itself. The apps that run the tests need virtualenvs from the same set, so this commit moves it to nix/python.nix, and flake.nix passes it to the checks. Signed-off-by: Nic Cope <nicc@rk0n.org>
The e2e tests are moving to pytest, and one test runner for the whole repo is simpler than two. pytest 9 runs the existing unittest suites unchanged and reports each subTest on its own, so this commit switches the runner without touching a test. It configures pytest in strict mode, to list each test, and to print whole assertion diffs, since the tests compare whole responses. nix run .#test runs every function's unit tests outside the sandbox, against the virtualenvs the checks use, or one function's with pytest arguments after its name. Towards modelplaneai#473. Signed-off-by: Nic Cope <nicc@rk0n.org>
The previous commit runs the unittest suites under pytest unchanged. This commit ports them to pytest's own style, so each case in a table is a test of its own that -k can select. Async tests call RunFunction with asyncio.run rather than needing a plugin. pytest captures a test's output and shows it only when the test fails, so the tests no longer disable the functions' logging. pytest diffs two dicts in insertion order, where unittest sorted their keys. The Structs a function returns rarely list their keys in the same order as the test's expected ones, so the tests compare dicts with sorted keys, which keeps a diff to the fields that differ. No case's request or expected response changes. Towards modelplaneai#473. Signed-off-by: Nic Cope <nicc@rk0n.org>
The tests built their cases in several ways. Some derived a case from another by copying and mutating it, or patched a request after building it. Others built whole requests and responses with helpers, asserted on individual fields, or computed expected values with the code under test or the SDK. A reader often couldn't tell what a case checked without tracing code. Some case names also claimed conditions their input didn't set up, and as sentences they made long test IDs. This commit rewrites the tests to the rules CONTRIBUTING now states, and says why in a comment wherever a test departs from them. Helpers take everything that varies as a required keyword argument, because a defaulted argument had hidden that a case named for an unpinned replica was pinned. A field-level test became a case in its entry point's table, or was deleted where a case already sent the same request. Each case has a short CamelCase name, which is its test ID, and a one-sentence reason, which pytest prints when it fails. Every distinct RunFunction request and response is unchanged. Writing cases out in full takes the tests from about 22,000 lines to 46,000. Towards modelplaneai#473. Signed-off-by: Nic Cope <nicc@rk0n.org>
CONTRIBUTING's Tests section sets out how a function's unit tests are written, but only a reviewer checked that a test followed it. This commit adds a checker that turns the rules an AST can decide into a pass or fail: a table and the test that runs it, a case's fields, name and reason, the assertion message, copies, merges and computed child names, module-level objects other than tables, a helper's parameters, and test names. It lives in hack/ and uses only the standard library. A function-test-style flake check type-checks and runs it, and nix run .#fix now lints its directory. A test with a reason to depart from a rule escapes it on the reported line, as in "# noqa: MPT401 # why". That's the form ruff already reads, so ruff now treats MPT codes as external rather than unknown. A case name counts an acronym such as GKE as one word, because many cases name a cloud or protocol that way. The new check fails on five violations in the tests as they stand. Towards modelplaneai#473. Signed-off-by: Nic Cope <nicc@rk0n.org>
The local e2e was a shell script, e2e/run.sh. Most of it checked the running environment, with polling loops written out by hand, JSON and logs matched as text, and an exit at the first failed check, so one fault hid every check after it. This commit replaces run.sh with a pytest suite that makes the checks run.sh's --verify made, one test each, and brings the clusters up the way run.sh did. Each test waits only for what it needs, so a gateway that refuses every caller fails only the tests that need it to serve. nix run .#e2e keeps its flags, and gains --test, to rerun the tests without bringing the clusters up again. Four checks are stricter. The response must name the served model in its top-level model field, where a model key at any depth passed. An unclaimed model must get a 404, where any status but 200 passed. /v1/models must list the model by its exact name, where a substring of another name passed. And the usage record must carry the full endpoint name, where a prefix of it passed. Fixes modelplaneai#473. Signed-off-by: Nic Cope <nicc@rk0n.org>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Polling expires prematurely, and the style checker does not enforce several mechanical rules it is intended to gate.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Ports unit and end-to-end testing to pytest, standardizing test structure, execution, and CI validation.
Changes:
- Adds pytest-based Nix runners, linting, type checks, and test-style enforcement.
- Rewrites function tests into table-driven, whole-response assertions.
- Replaces shell-based e2e verification with Python orchestration, fixtures, and focused test modules.
| File | Description |
|---|---|
pyproject.toml |
Adds pytest/Kubernetes dependencies and configuration. |
nix/python.nix |
Centralizes the uv2nix Python package set. |
nix/checks.nix |
Adds pytest, style, and e2e type checks. |
nix/apps.nix |
Adds unit-test and Python e2e runners. |
hack/check_function_tests.py |
Enforces function-test structure. |
functions/compose-usages/tests/test_fn.py |
Ports usage tests to pytest. |
functions/compose-usages/tests/__init__.py |
Removes obsolete test package marker. |
functions/compose-telemetry-destination/tests/test_fn.py |
Ports telemetry destination tests. |
functions/compose-telemetry-destination/tests/__init__.py |
Removes obsolete test package marker. |
functions/compose-serving-stack/tests/test_stacks.py |
Reworks serving-stack tests for pytest. |
functions/compose-serving-stack/tests/__init__.py |
Removes obsolete test package marker. |
functions/compose-model-service/tests/__init__.py |
Removes obsolete test package marker. |
functions/compose-model-route/tests/__init__.py |
Removes obsolete test package marker. |
functions/compose-model-replica/tests/__init__.py |
Removes obsolete test package marker. |
functions/compose-model-endpoint/tests/test_fn.py |
Ports endpoint tests to pytest. |
functions/compose-model-endpoint/tests/__init__.py |
Removes obsolete test package marker. |
functions/compose-model-deployment/tests/test_semver.py |
Ports and expands semver tests. |
functions/compose-model-deployment/tests/__init__.py |
Removes obsolete test package marker. |
functions/compose-metric-mapping/tests/test_fn.py |
Ports metric mapping tests. |
functions/compose-metric-mapping/tests/__init__.py |
Removes obsolete test package marker. |
functions/compose-inference-gateway/tests/__init__.py |
Removes obsolete test package marker. |
functions/compose-inference-class/tests/test_fn.py |
Ports inference-class tests. |
functions/compose-inference-class/tests/__init__.py |
Removes obsolete test package marker. |
functions/compose-gke-cluster/tests/__init__.py |
Removes obsolete test package marker. |
functions/compose-eks-cluster/tests/__init__.py |
Removes obsolete test package marker. |
flake.nix |
Wires shared Python packages and test apps. |
e2e/wait.py |
Adds convergence polling support. |
e2e/test_telemetry.py |
Adds telemetry e2e coverage. |
e2e/test_routing.py |
Adds gateway routing tests. |
e2e/test_metering.py |
Adds usage-metering verification. |
e2e/test_cluster_gateway.py |
Tests cluster-gateway connection security. |
e2e/test_auth.py |
Tests caller authentication. |
e2e/run.sh |
Removes the former shell runner. |
e2e/README.md |
Documents the pytest workflow. |
e2e/manifests/30-inference-cluster.yaml |
Updates orchestration references. |
e2e/manifests/20-inference-class.yaml |
Updates orchestration references. |
e2e/manifests/10-inference-gateway.yaml |
Updates test-runner references. |
e2e/manifests/00-namespaces.yaml |
Updates bring-up documentation. |
e2e/lean-control-plane.yaml |
Updates orchestration documentation. |
e2e/kube.py |
Adds Kubernetes API helpers. |
e2e/gateway.py |
Adds gateway request and log helpers. |
e2e/environment.py |
Implements Python cluster orchestration. |
e2e/dra-example-driver.yaml |
Updates bring-up documentation. |
e2e/conftest.py |
Defines shared e2e fixtures. |
e2e/__init__.py |
Documents the e2e package. |
CONTRIBUTING.md |
Defines the pytest testing convention. |
.github/workflows/e2e.yml |
Updates CI documentation for pytest. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+43
to
+46
| if time.monotonic() - start + interval > timeout: | ||
| e.add_note(f"Still failing after waiting {timeout:.0f}s for {what}.") | ||
| raise | ||
| time.sleep(interval) |
| return out | ||
|
|
||
|
|
||
| def check_tests(tree: ast.Module) -> list[Violation]: |
This branch has not been deployed
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.
Description of your changes
Fixes #473.
#473 asks how we should write e2e tests, and the bake-off pointed at pytest. This PR moves the e2e tests and the function unit tests to pytest, so the repo has one test runner, and rewrites every unit test to one consistent pattern, which CONTRIBUTING's Tests section now describes.
To judge the pattern, read compose-model-cache's tests. No unit test's input or expectation changed: every distinct request and response is the same as before. Mutation testing ran about 18,000 injected bugs in the function code against main's tests and this branch's. The branch's tests catch about 1,400 more than main's, and miss one main's catch. A
function-test-styleflake check now enforces the pattern's mechanical rules.nix run .#testruns every function's unit tests. Name a function to run only its tests, and pass pytest arguments after it. Each case is a test of its own, with a short name:When a case fails, pytest prints a sentence saying what the case checks, then the whole response, with the expected lines as
-and the actual lines as+, the same way round as Go'scmp.Diff(want, got). Here's one case with an expected value broken on purpose:nix run .#e2e -- --verifybrings up both kind clusters as before, then runs the e2e tests, passing any further arguments to pytest:--testreruns the e2e tests against clusters that are already up, and--cleantears them down:I have:
nix flake check(or./nix.sh flake check) and made sure it passes.Added or updated tests covering any composition function changes.This PR changes only tests.git commit -s.