Skip to content

chore: add a unittest template for testing bindings#105

Open
ryanking13 wants to merge 3 commits into
mainfrom
bindings-test
Open

chore: add a unittest template for testing bindings#105
ryanking13 wants to merge 3 commits into
mainfrom
bindings-test

Conversation

@ryanking13
Copy link
Copy Markdown
Contributor

This adds a test template for testing bindings such as r2, kv, d1.

I'll start adding tests for those bindings as a follow-up. This PR is mostly a preparation with a small smoke test using kv.

@ryanking13 ryanking13 requested review from dom96, hoodmane and joesepi May 14, 2026 13:33
Copy link
Copy Markdown

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm Bonk, and I've done a quick review of your PR.

This PR adds a test harness for exercising Cloudflare bindings (KV, R2, D1, etc.) by running an in-worker test suite via a live pywrangler dev server and mapping results back to pytest.

Issues found (highest severity first):

  1. Test names are double-prefixed with test_KV_TESTS contains "test_put_and_get", but both _make_test and the setattr call add another test_ prefix, producing test_test_put_and_get as the pytest method name. See inline suggestion.

  2. Each pytest test case re-runs the entire in-worker suite_run_suite() is called independently by every generated test method. With N tests this means N full HTTP round-trips executing the same suite. Consider caching the results per suite (e.g. with a session/module-scoped fixture or functools.lru_cache).

  3. Zombie process after process.kill() — In the dev_server fixture teardown, if process.wait(timeout=10) times out, process.kill() is called but process.wait() is not called afterward, which can leave a zombie process.

Comment thread packages/cli/tests/test_bindings.py Outdated
Comment thread packages/cli/tests/test_bindings.py
Comment thread packages/cli/tests/test_bindings.py Outdated
@ask-bonk
Copy link
Copy Markdown

ask-bonk Bot commented May 14, 2026

I'm Bonk, and I've done a quick review of your PR.

I posted a review on PR #105 with 3 issues found:

  1. Double test_ prefix on generated test namesKV_TESTS contains "test_put_and_get" but both _make_test and the setattr call add another test_ prefix, producing test_test_put_and_get. Posted a suggestion to drop the prefix from the list entries.

  2. Redundant suite execution — Each generated pytest test method calls _run_suite() independently, so the full in-worker suite runs N times for N tests. Suggested caching via lru_cache or a module-scoped fixture.

  3. Zombie process after process.kill() — In the fixture teardown, if process.wait(timeout=10) times out, process.kill() is called without a subsequent process.wait(), which can leave a zombie. Posted a suggestion fix.

github run

Copy link
Copy Markdown
Contributor

@dom96 dom96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting approach. I suppose we need to run wrangler to be able to use the bindings. I wonder if we should freeze the workerd/wrangler version we run these tests against, to ensure our CI is less susceptible to the world breaking it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants