fix(django-cf): run the django-cf suite in CI - #187
Merged
Conversation
ryanking13
requested review from
G4brym,
abstractedfox,
dom96,
hoodmane and
joesepi
August 6, 2026 11:11
|
Good - the R2 server does have Now let me verify the The AGENTS.md file at Let me also check if there's a potential TOCTOU issue with the port binding: The I've done a thorough review. The PR is clean - it correctly:
LGTM |
dom96
approved these changes
Aug 10, 2026
ryanking13
force-pushed
the
gyeongjae/django-cf-fix
branch
from
August 10, 2026 15:53
7561831 to
1bacdc7
Compare
ryanking13
force-pushed
the
gyeongjae/django-cf-test
branch
from
August 10, 2026 15:53
be2ae45 to
9ccb6c4
Compare
ryanking13
force-pushed
the
gyeongjae/django-cf-fix
branch
from
August 10, 2026 16:00
1bacdc7 to
3c6adff
Compare
ryanking13
force-pushed
the
gyeongjae/django-cf-test
branch
from
August 10, 2026 16:00
9ccb6c4 to
f43dc8e
Compare
ryanking13
force-pushed
the
gyeongjae/django-cf-fix
branch
from
August 11, 2026 04:07
3c6adff to
e5e744e
Compare
ryanking13
force-pushed
the
gyeongjae/django-cf-test
branch
from
August 11, 2026 04:07
f43dc8e to
060234e
Compare
The runtime does not convert binding results uniformly. D1's `stmt.all()` comes back dict-like, so `response.meta.rows_read` and `response.results.to_py()` were reading attributes that are not there, and `stmt.raw()` is already a list so calling `.to_py()` on it was wrong too. R2's `arrayBuffer()` yields a `memoryview`, which needs `bytes()` rather than a conversion call. Each site here was confirmed against a live Worker by printing the actual type, not inferred from the SDK's conversion rules, because those rules vary per call site: `rpc.py` passes unknown host classes through untouched while converting object literals, so whether a result is a `dict`, a `JsProxy` or a `memoryview` depends on what the binding returns. The three `head()` call sites in `storage/r2.py` keep `.to_py()` deliberately - R2Object is a host class and is genuinely still a JsProxy. The error handlers in `run_query` are left alone here; they are dealt with separately.
django-cf had no CI job, and a bare pytest could not stand in for one: it collected tests/d1, tests/durable_objects, tests/e2e, tests/r2 and tests/test_date_trunc.py, all of which drive a real `wrangler dev` through tests/utils.py and need a manually prepared Node toolchain. Only 130 of 208 tests could run unattended. Add a django-test job and give it fixtures that need no manual setup. 192 tests now run, up from 130. The dev dependencies move from [project.optional-dependencies] to [dependency-groups], because a plain `uv sync` does not install extras and the job would otherwise start without pytest. package.json's setup-test drops to `uv sync` for the same reason. tests/conftest.py ports the dev-server fixtures from runtime-sdk, replacing the re-imported helpers in tests/utils.py. Each fixture copies its Worker project to a temp directory, runs pywrangler sync, overwrites the vendored django-cf with the working tree and serves it on a free port. Copying per run removes the `npm run setup-test` snapshot step, whose stale copies could silently test old library code, and resolving the CLI from packages/cli via `uv run --with` means tests exercise the monorepo checkout rather than the released workers-py. Unlike the runtime-sdk original the servers run in uv project mode: each wrangler.jsonc has a build.command that runs collectstatic, which needs the project virtualenv on PATH, so --no-project fails before the Worker starts. The fixtures now assert that the migration and admin seeding endpoints succeeded; previously their responses were discarded, which is how a broken D1 backend stayed hidden. Startup failures dump the dev-server log. tests/e2e is excluded via addopts, for flakiness rather than tooling. It is the only suite that holds all three dev servers open at once, and about one run in three the D1 server exits mid-suite with nothing in its log, failing the rest with connection errors. Peak workerd RSS was 1.2 GB, so it is not memory pressure; the root cause is not yet identified. Without it the suite passed three consecutive runs. Run it deliberately with `pytest tests/e2e`. test_djangocf_durable_object_get_app_error_message read get_app.__code__.co_consts[1]. CPython 3.12 stopped emitting the leading None, so the index ran off the end and the test died with IndexError on main. It now asserts on the raised exception, which does not depend on bytecode layout. Moving fixture sharing into a real conftest.py drops the F811 per-file ignore, and start_new_session replaces preexec_fn, dropping PLW1509.
django-cf had two tiers of tests: 129 that ran on the host under plain CPython with mocked bindings, and the rest that exercised a real Worker. The host tier is the one that lies. Binding results come back as `dict` in one place, `JsProxy` in another and `memoryview` in a third, and the host mocks happily agreed with whichever shape the test author assumed. Settling those questions needed live instrumentation against a running Worker, not the test suite. So the host tier is gone. `tests/db/`, `tests/middleware/` and `tests/test_wsgi_handler.py` are replaced by `tests/in_worker/`, which follows the harness runtime-sdk already uses: pytest runs inside the worker, driven from the host by `register_in_worker_suites`. Everything is exercised against all three compat dates, matching the SDK, while the host stays on a single Python version. Coverage reconciles exactly: 459 tests now pass where 192 did before. That is 132 in-worker tests across 3 compat dates, plus the 63 Worker-backed tests that already existed. Nothing was dropped. The source-inspection tests were deleted rather than ported. They asserted on django-cf's source text, checking that a function's exception `handlers` list was empty or that a header expression appeared in `inspect.getsource`. They only existed because the real behaviour was not reachable from the host. Where they encoded a real intent it is now asserted directly: the header transformation is checked by sending actual headers through the real WSGI stack and reading them back out. Wall clock goes from 49s to about two minutes, against a 30 minute CI timeout. Cost is driven by workerd boots, which is files times compat dates, so the in-worker tests are deliberately consolidated into two files rather than mirroring the old layout. `tests/e2e/` stays excluded; it is flaky for unrelated reasons.
ryanking13
force-pushed
the
gyeongjae/django-cf-test
branch
from
August 11, 2026 06:16
060234e to
5878126
Compare
ryanking13
changed the title
tests/in_worker_harness.py and tests/in_worker/conftest.py were a second copy of what tests/conftest.py already did: two ways to start `pywrangler dev`, two readiness probes, two teardowns. They are now one file. The two start-up modes are not interchangeable and both survive - the app fixtures need uv project mode for their collectstatic build step, the in-worker fixture needs --no-project plus hand-vendored libraries - but they now share port allocation, the readiness poll and the process-group kill that keeps workerd from being orphaned. `dev_server` becomes `in_worker_server` and points at tests/in_worker/worker directly, which retires the `worker_project_dir` indirection. Python 3.12 leaves the in-worker matrix. That runtime (Pyodide 0.26.0a2) has no JSPI, so pyodide.ffi.run_sync is missing and all 15 R2Storage tests skip themselves; the rest duplicates the 3.13 and 3.14 runs. 505 collected tests become 348 and tests/in_worker drops from ~50s to ~33s. This is not the runtime-sdk's reason for excluding 3.12: these suites contain no async tests, so there were no false passes to fix here. The npm setup-* scripts go too. The harness copies django_cf into a tmpdir per run, so nothing has to be staged into templates/ or tests/servers/ first, and AGENTS.md no longer documents a step that does not exist. 346 passed, 2 skipped. Both skips are the strict xfail in tests/in_worker/worker/src/test_db.py, one per remaining config.
The in-worker migration merged six host test modules into two, and renamed a
lot of what it moved, which made it impossible to check the port against main
one test at a time. Split them back to the names main used:
test_db.py -> test_base_engine.py, test_d1_backend.py,
test_do_backend.py
test_middleware_wsgi.py -> test_cloudflare_access.py, test_storage_errors.py,
test_wsgi_handler.py
Bodies are moved verbatim; only files, class names and test names change. Each
file carries its own helpers rather than importing a shared module, matching
how the originals looked. Filenames are the suite names the in-worker runner
dispatches on, so this also splits the run into six /run-tests/<suite> calls
per config instead of two, and generated host-side classes are now CamelCase
(TestBaseEngine, not TestBASE_ENGINE).
159 tests on main, 156 now. test_base_engine.py matches main exactly, 68 for
68. The rest:
- 4 source-inspection tests are gone, and should be. They asserted on the text
of the implementation - that run_query uses `except Exception`, that
Error.stackTraceLimit is set, that storage.py contains an import. None
survive contact with a real runtime and none tested behaviour.
- 4 R2 tests that mocked a raising bucket are gone, replaced by real-binding
equivalents against real missing objects: *_on_exception became
*_on_not_found. Same intent, different mechanism, so the names changed.
- 2 host-only tests are gone: get_bucket_raises_on_non_worker cannot fail that
way inside a worker, and the two WSGI header-transformation tests inspected
handle_wsgi internals the Worker boundary tests now cover end to end.
- 3 genuinely uncovered: size when metadata has no size attribute, save
preserving content_type, and the mock-only read-raises path.
- 7 added: real D1 read/write round trips, DO storage wiring, and Access user
provisioning, none of which had host-side counterparts.
Full per-test mapping in /tmp/mig/report_db.md and /tmp/mig/report_middleware.md
(not committed).
348 passed, 2 skipped.
Contributor
Author
|
I ended up refactoring the test suite a bit to align with our current unittest structure. Let me clean things up as a follow up but merging for now so that other PRs can land too. |
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
2 participants
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.
Runs existing django-cf tests in CI. We will probably need to adopt some of the test fixtures from runtime-sdk as a follow up.