Refactor/colocate unit tests - #20
Merged
Merged
Conversation
Adds the tests/ and conftest.py exclusion patterns, a config tripwire test, and a CI step that asserts the built wheel itself. Also promotes _DEFAULT_CLIENT_CONFIG to a public DEFAULT_CLIENT_CONFIG so the per-tree test-client twins can share it, and adds the name-set checker that guards the colocation move against dropped tests.
Proves the pipeline end to end on the smallest module: a test written once under _async/ generates a working sync twin, and both run. make_client lives in _async/_testing.py rather than in the published glpi_python_client.testing module because its return type has to change when the tree is transformed, and testing.make_client is documented API that must keep returning a GlpiClient.
Also pulls test_4xx_raises_a_typed_status_error_from_ensure_response_status across from test_retry_semantics.py -- it exercises ensure_response_status directly and never touches the transport.
Retry policy, network-fault translation and timeout narrowing all live on the transport mixin, so their tests belong beside it. The stub callables become named async defs: the transport awaits them, and a lambda returning a FakeResponse cannot be awaited.
The docstring said "asynchronous GLPI transport mixin" and named AsyncGlpiClient specifically. Copied verbatim into _sync/, that read as false there: the generated file exercises GlpiClient synchronously and never touches AsyncGlpiClient. Reworded to describe what the tests assert without naming a surface-specific class, per C6.
Moves the 10 remaining client-lifecycle tests out of glpi_python_client/tests/clients/test_glpi_client.py into _async/clients/tests/test_client.py, rewritten as async def per the unasync recipe, and deletes the now-empty source file. Also tightens the generated-tree guard in test_unasync_codegen.py: it flagged the bare substring "_async" anywhere in a line, which false-positives on the two carried-over test names that legitimately contain "_async" as a fragment inside a larger identifier (test_glpi_client_async_context_manager, test_async_transport_ensure_open_blocks_after_close). The check now matches "_async" as its own token, matching what the codegen itself can and cannot rewrite, and keeps catching genuine qualified or bare tree references. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
test_glpi_client_init_failure_creates_no_session's docstring named httpx.AsyncClient and "the async surface" as the reason the old except-clause unwind was replaced with up-front validation. unasync only substitutes a string literal when its entire content is a substitution key, so that prose survived unrewritten into the generated sync twin, where it names a class and a surface absent from that tree -- a repeat of the C6 docstring defect from Task 5. Reworded to describe the constructor invariant without naming either client class or surface, matching the two docstrings already reworded in this file for the same reason. Assertion and test name unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds TransportRecorder and FailingTransportRecorder to _async/_testing.py so the recorder stub is written as async once rather than duplicated across nine mixin test files.
…_page Review found the test had been rewritten to stub the transport via TransportRecorder instead of client.search_tickets, dropping the call_count assertion, the "status==1" filter argument, and weakening len(batches[0]) == 1 to batches[0][0].id == 1. Restored the original structure (fake_search converted to async def per recipe C2), mirroring the sibling multi-page test.
test_api_coverage.py and test_smoke.py are fully dissolved: every test in them exercised one mixin, and none of them asserted contract completeness -- that is test_method_invocation.py's job.
test_kb_failure_paths.py unrolls into the four mixins it covered. The revision mixin is read-only, so it takes only the read suite.
…tests Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test_async_surface.py becomes _async/tests/test_concurrency.py, and the sync side gets a hand-written counterpart that asserts what is actually true there: ordered sequential gather and a real threading lock. Nothing exercised _sync/_concurrency.py directly before this.
test_the_lock_serialises_concurrent_threads had a critical section too short to create real contention: verified empirically that a no-op lock stand-in passed the test on every trial without a hold, since thread startup and GIL scheduling alone kept the eight runs from overlapping. A brief time.sleep in the critical section makes a broken lock fail every time while a real threading.Lock keeps passing.
Three of them derived the repo root with parents[2] from one level up and needed parents[3]. test_raise_site_audit.py did not: its parents[2] points at the package root, which is the same at both depths.
_build_module inserted _REPO_ROOT on sys.path and imported unasync_build, but pytest already puts the real repository root there, and sys.modules may hold the module from an earlier test. Either route satisfied the import regardless of _REPO_ROOT, so test_the_substitution_map_is_reachable -- the positive control meant to catch a broken root -- passed against one. Assert the imported module's __file__ resolves under _REPO_ROOT. With the root deliberately set one level too shallow the file now fails 6 tests including that control; before, it failed 4 and the control passed.
One rule: a unit test lives beside the module it tests. Records the two traps the async-source style introduces -- a lambda stub that cannot be transformed, and a forgotten await that passes vacuously.
CLAUDE.md is gitignored and has never been tracked, so the docstring added in the previous commit referred to a file no clone has. docs/development.md carries the same layout description and is tracked.
The colocation move is complete and every baseline name is still collected, so the checker has done its job.
test_glpi_client_async_context_manager traded a behavioural assertion for
a private flag during the move. Restored, and pinned to the guard's own
message: with _ensure_open disabled the call still raises RuntimeError,
but from httpx ("Cannot send a request, as the client has been closed."),
so match="closed" passed either way. It now fails on both surfaces.
Two docstrings were false once copied into the generated tree -- test_http
called the transport "asynchronous", and _testing.py claimed to be the
source that generates its twin, which also shipped in the wheel.
HAND_WRITTEN matched bare filenames at any depth, so a future
_async/**/test_concurrency.py would silently get no twin with --check
still green. Now tree-relative. Colocated tests make generic names recur,
so this stopped being hypothetical.
The raise-site positive control used endswith, which a root resolved one
level too deep still satisfies. Now pins the root and both exact paths.
CHANGELOG records that the wheel and sdist were shipping 56 test modules.
Five docstrings named an async-only spelling that unasync cannot rewrite -- substitution only fires when a string literal's entire content is a key, so a name inside a multi-sentence docstring passes through and the generated tree documents itself in terms that are false there. Two of them rendered into the published API reference: __enter__ was documented as returning AsyncGlpiClient, and a httpx.Client parameter as httpx.AsyncClient. All five are now surface-neutral or explicitly contrastive, and a new guard scans the generated tree for the other fifteen substitution keys the _async scan never covered. iter_search_tickets had no test pinning its pagination: mutating start += batch_size to start += 0, or dropping the sort= and fields= forwarding, all survived. Each now fails on both surfaces. The coverage omit matched glpi_python_client/testing/*, which excluded utils.py and fixtures.py as well as the suites -- published API measured by nothing. Narrowed to testing/tests/, and the helpers it exposed are now tested rather than merely counted: SearchResponse, TicketResponse and the client_factory fixture had shipped with no test at all. testing/ goes to 100%, total coverage 97.02% -> 97.07%. pytest stays out of the runtime dependencies and in the dev extra, where it already was. testing/fixtures.py is the one shipped module importing it, so it now says so and points application code at testing.utils.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20 +/- ##
==========================================
+ Coverage 96.95% 97.10% +0.15%
==========================================
Files 74 79 +5
Lines 2330 2422 +92
==========================================
+ Hits 2259 2352 +93
+ Misses 71 70 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… sync one The omit list excluded _async/ and measured the generated tree. That was the only tree the suite exercised before this branch, but it is the wrong one to report on: _async/ is what a person edits, so an uncovered line there is a line nobody tested, while an uncovered line in _sync/ only records which surface a test happened to run on. Flipping the omit exposed a real gap rather than just renaming the denominator. test_method_invocation.py swept all 85+ methods on GlpiClient but sampled three on AsyncGlpiClient -- a split that was harmless while _sync/ was measured and hid five async statements behind their sync twins the moment it was not. The async sweep now mirrors the sync one, driving async generators with `async for` rather than awaiting them. Net effect: 854 -> 935 tests, and coverage of the source tree is higher than the generated tree ever reported (97.11% vs 97.07%). The remaining async/sync asymmetry is one module, auth.py, where _async/ is better covered by one statement -- an async-only path in test_concurrency.py.
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.
Colocate unit tests beside the modules they test
Every unit test now lives in a
tests/package next to the module it covers —_async/auth/auth.pyis tested by_async/auth/tests/test_auth.py. Client tests are written once, asasync def, under_async/, andunasync_build.pygenerates the_sync/**/tests/twins, so both surfaces run the same assertions from a single source. Cross-cutting suites (codegen guards, reference guards, the raise-site audit, packaging) live inglpi_python_client/testing/tests/.integration_tests/is untouched.This also fixes a live packaging bug
mainships the test suite inside the distributions. Verified by building at both commits and diffing the file lists:[tool.hatch.build] excludenow dropstests/andconftest.py. The only additions anywhere in the distribution are the two_testing.pyhelpers; every removal is a test file.glpi_python_client.testing— the documented downstream factories, fixtures and fake responses — still ships in full, and the package imports cleanly in a venv with nopytestinstalled.Verification
-Wclean.test_kb_failure_pathsdissolved across 4 files andtest_api_coverageacross 10; old-union vs new-union is set-identical, no parameter dropped or duplicated.mainbut surviving here.Review findings, fixed
test_glpi_client_async_context_managerhad tradedpytest.raises(RuntimeError, ...)for a private_closedflag. Restored — and pinned to the guard's own message, because with_ensure_opendisabled the call still raisesRuntimeError, just from httpx ("Cannot send a request, as the client has been closed."), so the obviousmatch="closed"passed either way.__enter__documented as returningAsyncGlpiClient, and ahttpx.Clientparameter documented ashttpx.AsyncClient. A new guard now scans the generated tree for all fifteen remaining substitution keys, with an explicit allowlist for the deliberately contrastive sentences.HAND_WRITTENmatched bare filenames at any depth, so a future_async/**/test_concurrency.pywould silently get no twin while--checkstayed green — the scratch tree and_sync/would agree on its absence. Now tree-relative. Colocated tests make generic filenames recur, so this stopped being hypothetical.iter_search_ticketspagination was unasserted. Mutatingstart += batch_sizetostart += 0, or dropping thesort=/fields=forwarding, all survived. Each now fails on both surfaces.omitmatchedglpi_python_client/testing/*, excluding the published helpers as well as their suites. Narrowed — and the helpers it exposed are now tested rather than merely counted:SearchResponse,TicketResponseand theclient_factoryfixture had shipped with no test at all.testing/goes to 100%.sys.pathentry satisfied the import; another usedendswith, which a root resolved one level too deep still matches. Both now fail in the drift directions they were written for.Each fix was verified by breaking the thing it guards and watching the test fail, then restoring.
Known, pre-existing, not addressed here
lxmlis declared as a runtime dependency but never imported;markdownifyis called withoutfeatures=, so bs4's default parser is used.testing/fixtures.pyships and importspytest, which is correctly dev-only. The module now documents that and points application code attesting.utils.