chore: add smoke tests for packages that have entropy patches#133
chore: add smoke tests for packages that have entropy patches#133ryanking13 wants to merge 1 commit into
Conversation
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| import jiter | ||
| import pytest | ||
| import tiktoken._tiktoken | ||
| from cryptography.hazmat.primitives import hashes | ||
| from cryptography.hazmat.primitives.hashes import SHA256 | ||
|
|
||
| RUST_PACKAGES = ["tiktoken._tiktoken", "cryptography.exceptions", "jiter"] | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "order", | ||
| list(itertools.permutations(RUST_PACKAGES)), | ||
| ids=[ | ||
| "->".join(m.split(".")[0] for m in perm) | ||
| for perm in itertools.permutations(RUST_PACKAGES) | ||
| ], | ||
| ) | ||
| def test_rust_import_permutations(order): | ||
| for module_name in order: | ||
| __import__(module_name) |
There was a problem hiding this comment.
The top-level imports on lines 13-17 (import jiter, import tiktoken._tiktoken, from cryptography...) mean all three Rust packages are already in sys.modules before any test function runs. __import__() on an already-imported module is a no-op (it returns the cached module), so this parametrized test doesn't actually exercise different import orderings — every permutation does exactly the same thing.
If the goal is to verify that these packages survive the snapshot cycle (imported during snapshotting, usable after restore), the other tests in this file (test_tiktoken_import, test_cryptography_hashing, test_jiter_parse) already cover that. Consider either:
- Removing the permutation test since the other tests already cover snapshot survival, or
- Moving the Rust package imports out of module scope and into the test body using
importlibwithsys.modulescleanup if you truly want to test import ordering (though this may be impractical inside workerd).
As-is, the test passes but is misleading about what it validates.
|
I'm Bonk, and I've done a quick review of your PR. This PR adds smoke tests for packages that rely on entropy patches, verifying they survive the workerd snapshot save/restore cycle. The overall structure follows existing conventions well (matches the I flagged one issue:
|
|
|
||
|
|
||
| def test_aiohttp_http_websocket_import(): | ||
| assert aiohttp.http_websocket is not None |
There was a problem hiding this comment.
Does this do anything? Why would this ever be none?
| ) | ||
| def test_rust_import_permutations(order): | ||
| for module_name in order: | ||
| __import__(module_name) |
There was a problem hiding this comment.
This all runs in the same runtime so it's useless.
| import jiter | ||
| import pytest | ||
| import tiktoken._tiktoken | ||
| from cryptography.hazmat.primitives import hashes | ||
| from cryptography.hazmat.primitives.hashes import SHA256 | ||
|
|
||
| RUST_PACKAGES = ["tiktoken._tiktoken", "cryptography.exceptions", "jiter"] | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "order", | ||
| list(itertools.permutations(RUST_PACKAGES)), | ||
| ids=[ | ||
| "->".join(m.split(".")[0] for m in perm) | ||
| for perm in itertools.permutations(RUST_PACKAGES) | ||
| ], | ||
| ) | ||
| def test_rust_import_permutations(order): | ||
| for module_name in order: | ||
| __import__(module_name) |
2 participants
Since we are patching these packages. I think it should be useful for us to have test coverage so we don't suddenly break these packages. I found that in newer pydantic-core version (which is available in Python 3.14), our entropy patch doesn't work (#132). I believe these tests will allow us catching those errors when 1) we update Python versions, or when 2) those packages release a new version with breaking changes in entropy.