test: add quick_timing fixture and apply to register-heavy tests#1678
Merged
Conversation
Recent bump of _CHECK_TIME from 175ms to 500ms (made for RFC 6762 interop on slow WiFi / IoT networks) inflated every test that does real probe/announce/unregister by ~1-2s. The production constants are not needed for loopback tests — services register and answer themselves over 127.0.0.1 with no contention. Add an opt-in `quick_timing` fixture in tests/conftest.py that patches `_core._CHECK_TIME`, `_core._REGISTER_TIME`, and `_core._UNREGISTER_TIME` to 10ms each. Patching at the `_core` module is required because these constants are pulled in via `from .const import ...` at module load time, so patching `const.*` does not affect the binding `_core` already holds. Apply to seven of the worst-offending register-heavy tests, also passing explicit `timeout=200` to the post-unregister `async_get_service_info` calls and the patched-_is_complete `async_request` call so they don't pay the default 3000ms. Combined wall time across the seven tests drops from ~46s to ~7.5s: test_shutdown_while_register_in_process 10.69s -> 0.40s test_service_info_async_request 9.82s -> 0.7s test_async_zeroconf_service_types 6.16s -> 4.3s test_async_unregister_all_services 5.99s -> 0.52s test_async_service_registration_same_server_same_ports 3.23s -> ~0.2s test_async_service_registration_same_server_different_ports 3.18s -> ~0.2s test_async_service_registration_name_strict_check 2.99s -> ~0.2s The fixture is opt-in (not autouse) because a handful of tests under tests/test_handlers.py (`test_response_aggregation_timings`, `test_response_aggregation_timings_multiple`) assert against the production timing constants and must not be patched.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1678 +/- ##
=======================================
Coverage 99.76% 99.76%
=======================================
Files 33 33
Lines 3401 3401
Branches 461 461
=======================================
Hits 3393 3393
Misses 5 5
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The previous commit shortened two timeouts in this test (`async_request(zc, 3000)` and the post-unregister `async_get_service_info(..., timeout=200)`). The first one is the patched `_is_complete = False` block — the in-tree comment notes this exercises a TOCTOU race that 'is almost impossible to generate without patching.' Shortening the timeout shrinks the TOCTOU window enough that on slower CI runners (ubuntu 3.10 skip_cython, windows 3.9 skip_cython) the listener resolves to a stale pre-update broadcast (10.0.1.2) before the post-update record (10.0.1.3) arrives, and the assertion fails deterministically. Revert both timeouts to their defaults. The `quick_timing` fixture stays — it still cuts the register/update phases from ~6s to ~0.5s, so the test goes from ~9.82s to ~6.6s (3.3s saved) without reintroducing the race.
The previous revert kept the production timeouts but left quick_timing in place. CI still failed on 3.14t macOS skip_cython with the same assertion (`addresses == [10.0.1.2]` instead of `[10.0.1.3]`) — so the accelerated probe/announce intervals themselves race with the post-update broadcast on slower / pure- Python runners. With `_REGISTER_TIME=10ms`, the new info's announces can collide with still-in-flight broadcasts of the original (10.0.1.2) record and the listener ends up with the stale address. The test is exercising a TOCTOU race that needs the production timing to behave deterministically. Drop the fixture from this test entirely; it goes back to ~9.82s. The other six tests in the PR still benefit from quick_timing.
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
1 participant
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.
Summary
Recent bump of
_CHECK_TIMEfrom 175ms to 500ms — made forRFC 6762 interop on slow WiFi / IoT networks — inflated every
test that does real probe/announce/unregister by ~1–2s. The
production constants are not needed for loopback tests; services
register and answer themselves over 127.0.0.1 with no contention.
Adds an opt-in
quick_timingfixture and applies it (plus a fewshorter explicit timeouts) to seven of the worst-offending
register-heavy tests. Combined wall time drops from ~46s to ~7.5s.
Details
tests/conftest.pyNew
quick_timingfixture patches three module-level constantson
zeroconf._core:_CHECK_TIME_REGISTER_TIME_UNREGISTER_TIMEPatching at the
_coremodule is required because these areimported via
from .const import …at module load time, so apatch.object(const, …)does not rebind the name_corealreadyholds. Same reason
disable_duplicate_packet_suppressionpatchesconstdirectly — both styles already coexist in this file.The fixture yields nothing; the type hint is
Generator[None, None, None].Tests updated
Each of these now opts into
quick_timing(and where relevant,passes
timeout=200to a request that would otherwise wait thedefault 3000ms):
tests/test_core.py::test_shutdown_while_register_in_processtests/test_asyncio.py::test_service_info_async_requesttests/test_asyncio.py::test_async_zeroconf_service_typestests/test_asyncio.py::test_async_unregister_all_servicestests/test_asyncio.py::test_async_service_registration_same_server_same_portstests/test_asyncio.py::test_async_service_registration_same_server_different_portstests/test_asyncio.py::test_async_service_registration_name_strict_checktest_async_zeroconf_service_typesonly drops to ~4.3s becauseits body still calls
AsyncZeroconfServiceTypes.async_find(..., timeout=2)twice — that 2s is the property under test and isnot changed.
Why opt-in instead of autouse
tests/test_handlers.py::test_response_aggregation_timingsandtest_response_aggregation_timings_multipleassert against theproduction timing constants and must not be patched. Other slow
register-heavy tests can opt in incrementally in follow-up PRs.
Test plan
poetry run pytest tests/test_asyncio.py::test_async_service_registration_same_server_different_ports tests/test_asyncio.py::test_async_service_registration_same_server_same_ports tests/test_asyncio.py::test_async_service_registration_name_strict_check tests/test_asyncio.py::test_async_zeroconf_service_types tests/test_asyncio.py::test_service_info_async_request tests/test_asyncio.py::test_async_unregister_all_services tests/test_core.py::test_shutdown_while_register_in_processruns in 7.48s combined (was ~46s).
test_response_aggregation_timings*are untouched — theystill see the production constants.