Grandiras/ATProto.NET
1
1
Fork 0
Code Issues Pull requests Projects Releases 9 Packages 8 Wiki Activity Actions

test: shared test infrastructure, network-free unit tests, fewer low-value tests #175

Merged
Grandiras merged 17 commits from issue-137-tests into main 2026-09-26 22:15:27 +00:00
Conversation 6 Commits 17 Files changed 77 +1653 -2668
Grandiras commented 2026-09-26 22:11:43 +00:00
Owner
Copy link

Part of #137 (first half: test infrastructure, low-value tests, core coverage).

What changed

Shared scripted HttpMessageHandler stub and XrpcTestClient

  • tests/ATProtoNet.Tests/TestSupport/HttpStub.cs: one scripted HttpMessageHandler. A route is keyed by its XRPC NSID (for /xrpc/{nsid}) or by the full path otherwise; On(route, ...) queues a response (the last one queued repeats), To(route) returns the requests a route saw, and an unscripted request throws instead of returning a default — so a test that forgot to script something fails loudly instead of silently passing against whatever the fallback happened to return. Added along the way, once real files needed them:
    • Fallback(...) — answers any request with nothing scripted, for tests that assert on a cross-cutting concern (headers, retries, timeouts, service-URL switching) across many endpoints without caring which one a given call names.
    • Delay — an artificial per-response delay, for client-side timeout/cancellation tests.
    • ClearRequests() — forgets requests recorded so far (a test that signs in before the calls it actually wants to assert on).
    • Thread-safe: request recording and route dequeuing are under one lock, needed once a migrated file exercised concurrent calls (parallel admin calls sharing one session refresh).
    • Reads a request's ContentType/ContentLength before buffering its body, since HttpContent.Headers.ContentLength can turn from null (a non-seekable stream sent chunked) into the buffered byte count once read — this broke a chunked-upload assertion during migration and is fixed at the stub level.
  • tests/ATProtoNet.Tests/TestSupport/XrpcTestClient.cs: a small fixture pairing an HttpStub with a preconfigured AtProtoClient.
  • Folded the Bsky-only ScriptedXrpcHandler (used by 10 files) into HttpStub and migrated its consumers.

Migrated to HttpStub (35 files total, replacing their own MockHttpMessageHandler/CapturingHandler/RecordingHandler/etc.): the 10 former ScriptedXrpcHandler consumers under Lexicon/App/Bsky/**, plus 25 more — AtProtoClientConstructionTests, RecordCollectionErrorTests, Admin/PdsAdminClientTests, Admin/PdsAdminClientAccountAuthTests, Http/XrpcClientProxyTests, Http/XrpcClientRepoRevTests, Http/XrpcClientRateLimitTests, Http/XrpcClientAdminAuthTests, Http/XrpcClientLabelerHeaderTests, Http/EmptyResponseBodyTests, Http/CustomXrpcTests, Http/XrpcTransportTests, Http/PaginationTests, Lexicon/WireFidelityTests, Lexicon/App/Bsky/TypedBskyClientTests, Lexicon/App/Bsky/Labeler/LabelerClientTests, Lexicon/Com/AtProto/ComAtProtoGapTests, Lexicon/Com/AtProto/IdentityClientTests, Lexicon/Site/Standard/StandardSiteClientTests, Lexicon/Com/AtProto/TypedRepoClientTests, and Chat/Bsky/{Actor,Notification,Moderation,Group,Convo}ClientTests (5 files). That accounts for the issue's "27 hand-rolled handlers" estimate and then some; the remainder below were deliberately not touched.

Deliberately left alone (already-shared, non-duplicated fixtures specialized enough that folding them into a generic stub would lose real capability, not gain consistency):

  • Auth/SessionTestKit.cs (StubServer) — serves 5 Auth/OAuth test files; understands OAuth metadata documents, DPoP proof claims and form-encoded token bodies, none of which HttpStub needs elsewhere.
  • Identity/IdentityTestSupport.cs (ScriptedHandler + a real loopback TcpListener-based server) — serves 3 Identity files; the loopback server exists specifically to exercise the real socket handler (truncated bodies, broken encodings) safely.
  • Ozone/OzoneTestClient.cs — serves 4 Ozone files with an IsQuery/IsProcedure assertion DSL.
  • BlueskyHelpersTests.cs (FakePds) and Auth/OAuth/OAuthCallbackHandleVerificationTests.cs (FlowStub) — stateful, single-file fakes (a small in-memory profile store with CAS-retry behaviour; a whole simulated OAuth deployment with revocation tracking) that don't map onto a per-route scripted queue without losing behaviour.

Not migrated, left for a follow-up (time-boxed; none of these are hand-rolled duplicates of the pattern the shared stub targets, and several sit next to the DI surface #136 is actively redesigning): Ozone/{OzoneClientTests,TypedOzoneClientTests}, Server/Authentication/AtProtoOAuthServiceHttpClientTests, the Spaces/** and Server/Spaces/** handler-based files (SpaceClientTests, SpaceCredentialMintTests, SpaceCredentialProviderTests, SpaceSyncerTests, HttpSpaceClientMetadataResolverTests, SimpleSpaceManagingAppClientTests, SpaceServerEndpointTests, SpaceServiceAuthKeyTests, SpaceWriteNotifierTests), Streaming/{JetstreamArchiveClientTests,JetstreamArchiveClientPublicEndpointTests,JetstreamReplayConsumerTests}, Sync/SyncClientTests (binary CAR fixtures), Tap/TapClientTests.

One XrpcTestHost

Server/XrpcEndpointTests.cs already had a well-built XrpcTestHost + CapturingLoggerProvider, reused by four other files (ServiceAuthSchemeTests, QueryLabelsEndpointTests, XrpcQueryBindingTests, XrpcRouteGroupTests) only because they sat in a descendant namespace — an implicit, fragile sharing mechanism. Moved both into tests/ATProtoNet.Tests/TestSupport/, alongside HttpStub, so they're found by an explicit using like every other shared fixture; added a mapXrpcEndpoints flag so a host that maps only its own endpoints (no IXrpcQuery/IXrpcProcedure registered) can opt out of the unconditional MapXrpcEndpoints(), which throws without one.

The other TestServer-based hosts (Server/Authentication/{AtProtoOAuthEndpointTests,ServiceAuthWithSpacesTests}, Server/Spaces/{SimpleSpaceEndpointTests,SpaceServerEndpointTests}) were left as their own bespoke HostBuilder/UseTestServer() setups: each wires up AddAtProtoServer/AddAtProtoSpaces/OAuth DI registration that #136 is actively redesigning, so consolidating them now risks conflicting with, or being redone by, that work. Server/TapWebhookTests.cs uses WebApplication.CreateSlimBuilder() deliberately (a minimal host for pure endpoint-routing tests) rather than the full generic Host; left as-is.

One enum-driven [RequiresFact] / [RequiresTheory]

RequiresPdsFactAttribute, RequiresBlueskyFactAttribute, RequiresPdsAdminFactAttribute, RequiresJetstreamFactAttribute, RequiresJetstreamArchiveFactAttribute and RequiresSpacesFactAttribute are replaced by one IntegrationRequirement enum and [RequiresFact(IntegrationRequirement.X)] / [RequiresTheory(IntegrationRequirement.X)], in tests/ATProtoNet.IntegrationTests/TestInfrastructure.cs. RequiresPdsFact and RequiresBlueskyFact used to ignore ATPROTO_REQUIRE_INTEGRATION and always skip when their prerequisite was missing; the unified attribute honours it like the others (the attribute lets the test run instead of skipping, and the test fails on its own for lacking what it needs — verified live: with ATPROTO_REQUIRE_INTEGRATION=1 and no PDS configured, AuthenticationTests now fails with a real connection/argument error instead of reporting 7 skips). RequiresRedisFactAttribute is untouched (#136 removes the Redis store). All ~90 attribute usages across tests/ATProtoNet.IntegrationTests/*.cs were updated; CONTRIBUTING.md, docs/testing-spaces.md and CLAUDE.md updated to match.

Unit tests must not touch the network

Auth/OAuth/SecurityTests.cs's SSRF tests (ResolveFromIdentifier_RefusedDidWeb_IsInvalidDidWithoutARequest etc.) built their IdentityResolver via IdentityResolver.CreateDefault(...), which owns its own real HttpClients for DID and handle resolution — entirely separate from the scripted handler passed to AuthorizationServerDiscovery. If the SSRF pre-check under test ever regressed, a refused identifier (169.254.169.254, 10.0.0.1, ...) could reach a real socket instead of the test's stub, from inside a unit test run. Rewired the DID/handle resolvers through the same scripted handler using PlcClient/DidWebResolver/HandleResolver's existing HttpClient-accepting constructors — no src/ change needed, the seam already existed. Also replaced two bare new HttpClient() instances (in tests that only validate a URI and never send anything) with a handler that throws if one ever does.

Audited the rest of tests/ATProtoNet.Tests for real DNS/socket/TCP usage (Dns.*, TcpClient, Socket, bare HttpClient()): the only other loopback socket usage (Identity/IdentityFetchPolicyTests.cs) already uses an owned TcpListener on IPAddress.Loopback, which is the sanctioned pattern for the one case (the SSRF connect-callback policy) that has to exercise a real socket. No AtProtoClientApplyOAuthTests.cs file exists in the current tree — the issue's brief evidence predates a rename or removal; nothing further to change there.

Removed tests that test nothing

An audit against the existing blanket coverage (Identity/IdentifierContractTests, the Lexicon drift/JSON-name tests, and the pinned interop fixtures for DAG-CBOR/CID/crypto) found 30 test methods (one whole file) that assert nothing beyond what that coverage, or a sibling test in the same file, already pins. None carry a bug/issue/regression comment and none assert a security property; every removal was verified against the specific test claimed to already cover it before deleting.

  • DTO getter/setter (6): OAuthModelsTests.{OAuthException_HasErrorCode,OAuthException_WithInnerException,JsonWebKey_Properties,JsonWebKeySet_Properties}, RecordCollectionTests.{AtProtoRecord_Type_ReturnsCorrectNsid,AtProtoRecord_CanSetCustomCreatedAt} — property echoes with no serialization or behaviour exercised.
  • Constant-equals-literal (1): RecordKeyTests.Self_IsValidSingleton — RecordKey.Self is already asserted (== (RecordKey)"self") by IdentifierContractTests.ExplicitConversionsFromString_ParseOrThrow.
  • Smoke tests asserting nothing meaningful (8): ChatClientsTests.cs (whole file — sub-client NotNull only), AtProtoClientConstructionTests.Constructor_SubClients_AreAccessible, StandardSiteClientTests.AtProtoClient_Site_IsAvailable, OzoneClientTests.Ozone_Property_Exists, DynamicPdsTests.{ServiceUrl_DefaultValue,Session_NullByDefault}, RecordCollectionTests.{AtProtoRecord_DifferentTypes_HaveDifferentNsids,RecordPage_CanContainMultipleRecords} — all Assert.NotNull/trivial-inequality/count checks that every real test of the same sub-client or type already exercises implicitly.
  • Round-trips subsumed by pinned vectors (15): IdentitySerializationTests.{Did,Nsid,AtUri,Tid,RecordKey}_RoundTrips (each type already has a *_Serializes_AsString pinning the exact wire string — kept Cid_RoundTrips, which additionally pins one); DagCborRoundtripTests.{Roundtrip_TypeField_Preserved,EncodeThenComputeCid_IsConsistent} (verbatim duplicate of DagCborEncoderTests.Encode_TypeDiscriminator_Preserved; determinism covered with pinned bytes+CID by DagCborInteropTests.Encode_DataModelFixture_MatchesReferenceBytesAndCid); MstNodeDataTests.ToBytes_IsDeterministic (pinned by the reference vectors and ToBytes_AbsentLinks_AreWrittenAsNull); CidComputationTests.{ComputeForDagCbor_ProducesValidCid,ComputeForDagCbor_IsDeterministic} (pinned by CidTests.ComputeForDagCbor_ReturnsParsedCid); DidTests.{ExplicitCast_FromString_Works,CompareTo_Orders_Correctly}, HandleTests.ExplicitCast_FromString_Works, TidTests.CompareTo_OrdersCorrectly, AtUriTests.CompareTo_IsOrdinal — all duplicate the shared IdentifierContractTests.AssertContract<T> helper, which every identifier type already runs through (CompareTo ordering both directions, CompareTo(null), and the explicit string cast).

Unit test count goes from 4993 (after the coverage addition above) to 4963, all still green. A "merge duplicate facts into one theory" pass (about 20 groups the audit identified, e.g. JetstreamEventParserTests's several Parse_*_ReturnsNull facts, AtProtoScopesTests's constant/scope-building facts) was scoped out of this PR to keep the diff reviewable; noted for a follow-up.

Coverage for core paths

  • AtProtoClient.LoginAsync/ResumeSessionAsync/LogoutAsync: already thoroughly covered by Auth/PasswordSessionLifecycleTests.cs (sign-in, resume with refresh, reactive/proactive refresh, sign-out, disposal) — checked first per the issue; nothing to add.
  • RecordCollection<T> CRUD: Create/Get/Find/Put/List/Exists/Enumerate were already covered (RecordCollectionErrorTests.cs, Http/PaginationTests.cs); DeleteAsync had no test through the typed wrapper (only via RepoClient.DeleteRecordAsync directly) — added DeleteAsync_SendsTheRepoCollectionAndKey and DeleteAsync_WithASwapRecord_SendsIt.
  • Custom XRPC entry points (QueryAsync/ProcedureAsync, IXrpcTransport): already covered by Http/CustomXrpcTests.cs (typed and anonymous-object parameters, void/parameterless overloads, IXrpcTransport used by a third-party-style sub-client, download/upload, NSubstitute fake for a sub-client's own tests) — migrated to HttpStub, no gap found.

Breaking changes

None to the published SDK. Test-project-internal rename: RequiresPdsFactAttribute / RequiresBlueskyFactAttribute / RequiresPdsAdminFactAttribute / RequiresJetstreamFactAttribute / RequiresJetstreamArchiveFactAttribute / RequiresSpacesFactAttribute → [RequiresFact(IntegrationRequirement.X)] (tests/ATProtoNet.IntegrationTests is not packed). RequiresRedisFactAttribute unchanged.

Verified

  • dotnet build -p:EnableSourceControlManagerQueries=false (full solution, Debug and Release): 0 warnings, 0 errors.
  • dotnet test tests/ATProtoNet.Tests/ -p:EnableSourceControlManagerQueries=false: before (start of this branch) 4991 passed / 5 skipped / 4996 total; after the DeleteAsync coverage addition, 4993 passed; after removing the 30 low-value tests, 4963 passed / 5 skipped / 4968 total, 0 failures (also verified in --configuration Release).
  • dotnet test tests/ATProtoNet.IntegrationTests/: 90 skipped locally (unchanged); with ATPROTO_REQUIRE_INTEGRATION=1 and no PDS configured, the PDS-gated tests fail instead of skipping (spot-checked AuthenticationTests, 7/7 now fail with a real connection/argument error).

Deferred to the second half (after #136 merges), per the issue's own split

Token-store/space-store contract base classes; EF tests on SQLite (dropping Microsoft.EntityFrameworkCore.InMemory); the integration-CI changes (AuthenticatedClientFixture provisioning, widening the CI filter). Also deferred, noted above: the remaining hand-rolled handlers in the Spaces/Streaming/Sync/Tap areas, and the four Space/OAuth TestServer hosts that overlap #136's DI redesign.

🤖 Generated with Claude Code

Part of #137 (first half: test infrastructure, low-value tests, core coverage). ## What changed ### Shared scripted `HttpMessageHandler` stub and `XrpcTestClient` - `tests/ATProtoNet.Tests/TestSupport/HttpStub.cs`: one scripted `HttpMessageHandler`. A route is keyed by its XRPC NSID (for `/xrpc/{nsid}`) or by the full path otherwise; `On(route, ...)` queues a response (the last one queued repeats), `To(route)` returns the requests a route saw, and an unscripted request **throws** instead of returning a default — so a test that forgot to script something fails loudly instead of silently passing against whatever the fallback happened to return. Added along the way, once real files needed them: - `Fallback(...)` — answers any request with nothing scripted, for tests that assert on a cross-cutting concern (headers, retries, timeouts, service-URL switching) across many endpoints without caring which one a given call names. - `Delay` — an artificial per-response delay, for client-side timeout/cancellation tests. - `ClearRequests()` — forgets requests recorded so far (a test that signs in before the calls it actually wants to assert on). - Thread-safe: request recording and route dequeuing are under one lock, needed once a migrated file exercised concurrent calls (parallel admin calls sharing one session refresh). - Reads a request's `ContentType`/`ContentLength` before buffering its body, since `HttpContent.Headers.ContentLength` can turn from `null` (a non-seekable stream sent chunked) into the buffered byte count once read — this broke a chunked-upload assertion during migration and is fixed at the stub level. - `tests/ATProtoNet.Tests/TestSupport/XrpcTestClient.cs`: a small fixture pairing an `HttpStub` with a preconfigured `AtProtoClient`. - Folded the Bsky-only `ScriptedXrpcHandler` (used by 10 files) into `HttpStub` and migrated its consumers. **Migrated to `HttpStub`** (35 files total, replacing their own `MockHttpMessageHandler`/`CapturingHandler`/`RecordingHandler`/etc.): the 10 former `ScriptedXrpcHandler` consumers under `Lexicon/App/Bsky/**`, plus 25 more — `AtProtoClientConstructionTests`, `RecordCollectionErrorTests`, `Admin/PdsAdminClientTests`, `Admin/PdsAdminClientAccountAuthTests`, `Http/XrpcClientProxyTests`, `Http/XrpcClientRepoRevTests`, `Http/XrpcClientRateLimitTests`, `Http/XrpcClientAdminAuthTests`, `Http/XrpcClientLabelerHeaderTests`, `Http/EmptyResponseBodyTests`, `Http/CustomXrpcTests`, `Http/XrpcTransportTests`, `Http/PaginationTests`, `Lexicon/WireFidelityTests`, `Lexicon/App/Bsky/TypedBskyClientTests`, `Lexicon/App/Bsky/Labeler/LabelerClientTests`, `Lexicon/Com/AtProto/ComAtProtoGapTests`, `Lexicon/Com/AtProto/IdentityClientTests`, `Lexicon/Site/Standard/StandardSiteClientTests`, `Lexicon/Com/AtProto/TypedRepoClientTests`, and `Chat/Bsky/{Actor,Notification,Moderation,Group,Convo}ClientTests` (5 files). That accounts for the issue's "27 hand-rolled handlers" estimate and then some; the remainder below were deliberately not touched. **Deliberately left alone** (already-shared, non-duplicated fixtures specialized enough that folding them into a generic stub would lose real capability, not gain consistency): - `Auth/SessionTestKit.cs` (`StubServer`) — serves 5 Auth/OAuth test files; understands OAuth metadata documents, DPoP proof claims and form-encoded token bodies, none of which `HttpStub` needs elsewhere. - `Identity/IdentityTestSupport.cs` (`ScriptedHandler` + a real loopback `TcpListener`-based server) — serves 3 Identity files; the loopback server exists specifically to exercise the real socket handler (truncated bodies, broken encodings) safely. - `Ozone/OzoneTestClient.cs` — serves 4 Ozone files with an `IsQuery`/`IsProcedure` assertion DSL. - `BlueskyHelpersTests.cs` (`FakePds`) and `Auth/OAuth/OAuthCallbackHandleVerificationTests.cs` (`FlowStub`) — stateful, single-file fakes (a small in-memory profile store with CAS-retry behaviour; a whole simulated OAuth deployment with revocation tracking) that don't map onto a per-route scripted queue without losing behaviour. **Not migrated, left for a follow-up** (time-boxed; none of these are hand-rolled duplicates of the pattern the shared stub targets, and several sit next to the DI surface #136 is actively redesigning): `Ozone/{OzoneClientTests,TypedOzoneClientTests}`, `Server/Authentication/AtProtoOAuthServiceHttpClientTests`, the `Spaces/**` and `Server/Spaces/**` handler-based files (`SpaceClientTests`, `SpaceCredentialMintTests`, `SpaceCredentialProviderTests`, `SpaceSyncerTests`, `HttpSpaceClientMetadataResolverTests`, `SimpleSpaceManagingAppClientTests`, `SpaceServerEndpointTests`, `SpaceServiceAuthKeyTests`, `SpaceWriteNotifierTests`), `Streaming/{JetstreamArchiveClientTests,JetstreamArchiveClientPublicEndpointTests,JetstreamReplayConsumerTests}`, `Sync/SyncClientTests` (binary CAR fixtures), `Tap/TapClientTests`. ### One `XrpcTestHost` `Server/XrpcEndpointTests.cs` already had a well-built `XrpcTestHost` + `CapturingLoggerProvider`, reused by four other files (`ServiceAuthSchemeTests`, `QueryLabelsEndpointTests`, `XrpcQueryBindingTests`, `XrpcRouteGroupTests`) only because they sat in a descendant namespace — an implicit, fragile sharing mechanism. Moved both into `tests/ATProtoNet.Tests/TestSupport/`, alongside `HttpStub`, so they're found by an explicit `using` like every other shared fixture; added a `mapXrpcEndpoints` flag so a host that maps only its own endpoints (no `IXrpcQuery`/`IXrpcProcedure` registered) can opt out of the unconditional `MapXrpcEndpoints()`, which throws without one. The other TestServer-based hosts (`Server/Authentication/{AtProtoOAuthEndpointTests,ServiceAuthWithSpacesTests}`, `Server/Spaces/{SimpleSpaceEndpointTests,SpaceServerEndpointTests}`) were left as their own bespoke `HostBuilder`/`UseTestServer()` setups: each wires up `AddAtProtoServer`/`AddAtProtoSpaces`/OAuth DI registration that #136 is actively redesigning, so consolidating them now risks conflicting with, or being redone by, that work. `Server/TapWebhookTests.cs` uses `WebApplication.CreateSlimBuilder()` deliberately (a minimal host for pure endpoint-routing tests) rather than the full generic `Host`; left as-is. ### One enum-driven `[RequiresFact]` / `[RequiresTheory]` `RequiresPdsFactAttribute`, `RequiresBlueskyFactAttribute`, `RequiresPdsAdminFactAttribute`, `RequiresJetstreamFactAttribute`, `RequiresJetstreamArchiveFactAttribute` and `RequiresSpacesFactAttribute` are replaced by one `IntegrationRequirement` enum and `[RequiresFact(IntegrationRequirement.X)]` / `[RequiresTheory(IntegrationRequirement.X)]`, in `tests/ATProtoNet.IntegrationTests/TestInfrastructure.cs`. `RequiresPdsFact` and `RequiresBlueskyFact` used to ignore `ATPROTO_REQUIRE_INTEGRATION` and always skip when their prerequisite was missing; the unified attribute honours it like the others (the attribute lets the test run instead of skipping, and the test fails on its own for lacking what it needs — verified live: with `ATPROTO_REQUIRE_INTEGRATION=1` and no PDS configured, `AuthenticationTests` now fails with a real connection/argument error instead of reporting 7 skips). `RequiresRedisFactAttribute` is untouched (#136 removes the Redis store). All ~90 attribute usages across `tests/ATProtoNet.IntegrationTests/*.cs` were updated; `CONTRIBUTING.md`, `docs/testing-spaces.md` and `CLAUDE.md` updated to match. ### Unit tests must not touch the network `Auth/OAuth/SecurityTests.cs`'s SSRF tests (`ResolveFromIdentifier_RefusedDidWeb_IsInvalidDidWithoutARequest` etc.) built their `IdentityResolver` via `IdentityResolver.CreateDefault(...)`, which owns its own real `HttpClient`s for DID and handle resolution — entirely separate from the scripted handler passed to `AuthorizationServerDiscovery`. If the SSRF pre-check under test ever regressed, a refused identifier (`169.254.169.254`, `10.0.0.1`, ...) could reach a real socket instead of the test's stub, from inside a unit test run. Rewired the DID/handle resolvers through the same scripted handler using `PlcClient`/`DidWebResolver`/`HandleResolver`'s existing `HttpClient`-accepting constructors — no `src/` change needed, the seam already existed. Also replaced two bare `new HttpClient()` instances (in tests that only validate a URI and never send anything) with a handler that throws if one ever does. Audited the rest of `tests/ATProtoNet.Tests` for real DNS/socket/TCP usage (`Dns.*`, `TcpClient`, `Socket`, bare `HttpClient()`): the only other loopback socket usage (`Identity/IdentityFetchPolicyTests.cs`) already uses an owned `TcpListener` on `IPAddress.Loopback`, which is the sanctioned pattern for the one case (the SSRF connect-callback policy) that has to exercise a real socket. No `AtProtoClientApplyOAuthTests.cs` file exists in the current tree — the issue's brief evidence predates a rename or removal; nothing further to change there. ### Removed tests that test nothing An audit against the existing blanket coverage (`Identity/IdentifierContractTests`, the Lexicon drift/JSON-name tests, and the pinned interop fixtures for DAG-CBOR/CID/crypto) found 30 test methods (one whole file) that assert nothing beyond what that coverage, or a sibling test in the same file, already pins. None carry a bug/issue/regression comment and none assert a security property; every removal was verified against the specific test claimed to already cover it before deleting. - **DTO getter/setter (6):** `OAuthModelsTests.{OAuthException_HasErrorCode,OAuthException_WithInnerException,JsonWebKey_Properties,JsonWebKeySet_Properties}`, `RecordCollectionTests.{AtProtoRecord_Type_ReturnsCorrectNsid,AtProtoRecord_CanSetCustomCreatedAt}` — property echoes with no serialization or behaviour exercised. - **Constant-equals-literal (1):** `RecordKeyTests.Self_IsValidSingleton` — `RecordKey.Self` is already asserted (`== (RecordKey)"self"`) by `IdentifierContractTests.ExplicitConversionsFromString_ParseOrThrow`. - **Smoke tests asserting nothing meaningful (8):** `ChatClientsTests.cs` (whole file — sub-client `NotNull` only), `AtProtoClientConstructionTests.Constructor_SubClients_AreAccessible`, `StandardSiteClientTests.AtProtoClient_Site_IsAvailable`, `OzoneClientTests.Ozone_Property_Exists`, `DynamicPdsTests.{ServiceUrl_DefaultValue,Session_NullByDefault}`, `RecordCollectionTests.{AtProtoRecord_DifferentTypes_HaveDifferentNsids,RecordPage_CanContainMultipleRecords}` — all `Assert.NotNull`/trivial-inequality/count checks that every real test of the same sub-client or type already exercises implicitly. - **Round-trips subsumed by pinned vectors (15):** `IdentitySerializationTests.{Did,Nsid,AtUri,Tid,RecordKey}_RoundTrips` (each type already has a `*_Serializes_AsString` pinning the exact wire string — kept `Cid_RoundTrips`, which additionally pins one); `DagCborRoundtripTests.{Roundtrip_TypeField_Preserved,EncodeThenComputeCid_IsConsistent}` (verbatim duplicate of `DagCborEncoderTests.Encode_TypeDiscriminator_Preserved`; determinism covered with pinned bytes+CID by `DagCborInteropTests.Encode_DataModelFixture_MatchesReferenceBytesAndCid`); `MstNodeDataTests.ToBytes_IsDeterministic` (pinned by the reference vectors and `ToBytes_AbsentLinks_AreWrittenAsNull`); `CidComputationTests.{ComputeForDagCbor_ProducesValidCid,ComputeForDagCbor_IsDeterministic}` (pinned by `CidTests.ComputeForDagCbor_ReturnsParsedCid`); `DidTests.{ExplicitCast_FromString_Works,CompareTo_Orders_Correctly}`, `HandleTests.ExplicitCast_FromString_Works`, `TidTests.CompareTo_OrdersCorrectly`, `AtUriTests.CompareTo_IsOrdinal` — all duplicate the shared `IdentifierContractTests.AssertContract<T>` helper, which every identifier type already runs through (`CompareTo` ordering both directions, `CompareTo(null)`, and the explicit string cast). Unit test count goes from 4993 (after the coverage addition above) to 4963, all still green. A "merge duplicate facts into one theory" pass (about 20 groups the audit identified, e.g. `JetstreamEventParserTests`'s several `Parse_*_ReturnsNull` facts, `AtProtoScopesTests`'s constant/scope-building facts) was scoped out of this PR to keep the diff reviewable; noted for a follow-up. ### Coverage for core paths - `AtProtoClient.LoginAsync`/`ResumeSessionAsync`/`LogoutAsync`: already thoroughly covered by `Auth/PasswordSessionLifecycleTests.cs` (sign-in, resume with refresh, reactive/proactive refresh, sign-out, disposal) — checked first per the issue; nothing to add. - `RecordCollection<T>` CRUD: `Create`/`Get`/`Find`/`Put`/`List`/`Exists`/`Enumerate` were already covered (`RecordCollectionErrorTests.cs`, `Http/PaginationTests.cs`); `DeleteAsync` had no test through the typed wrapper (only via `RepoClient.DeleteRecordAsync` directly) — added `DeleteAsync_SendsTheRepoCollectionAndKey` and `DeleteAsync_WithASwapRecord_SendsIt`. - Custom XRPC entry points (`QueryAsync`/`ProcedureAsync`, `IXrpcTransport`): already covered by `Http/CustomXrpcTests.cs` (typed and anonymous-object parameters, void/parameterless overloads, `IXrpcTransport` used by a third-party-style sub-client, download/upload, NSubstitute fake for a sub-client's own tests) — migrated to `HttpStub`, no gap found. ## Breaking changes None to the published SDK. Test-project-internal rename: `RequiresPdsFactAttribute` / `RequiresBlueskyFactAttribute` / `RequiresPdsAdminFactAttribute` / `RequiresJetstreamFactAttribute` / `RequiresJetstreamArchiveFactAttribute` / `RequiresSpacesFactAttribute` → `[RequiresFact(IntegrationRequirement.X)]` (`tests/ATProtoNet.IntegrationTests` is not packed). `RequiresRedisFactAttribute` unchanged. ## Verified - `dotnet build -p:EnableSourceControlManagerQueries=false` (full solution, Debug and Release): 0 warnings, 0 errors. - `dotnet test tests/ATProtoNet.Tests/ -p:EnableSourceControlManagerQueries=false`: before (start of this branch) 4991 passed / 5 skipped / 4996 total; after the `DeleteAsync` coverage addition, 4993 passed; after removing the 30 low-value tests, **4963 passed / 5 skipped / 4968 total, 0 failures** (also verified in `--configuration Release`). - `dotnet test tests/ATProtoNet.IntegrationTests/`: 90 skipped locally (unchanged); with `ATPROTO_REQUIRE_INTEGRATION=1` and no PDS configured, the PDS-gated tests fail instead of skipping (spot-checked `AuthenticationTests`, 7/7 now fail with a real connection/argument error). ## Deferred to the second half (after #136 merges), per the issue's own split Token-store/space-store contract base classes; EF tests on SQLite (dropping `Microsoft.EntityFrameworkCore.InMemory`); the integration-CI changes (`AuthenticatedClientFixture` provisioning, widening the CI filter). Also deferred, noted above: the remaining hand-rolled handlers in the Spaces/Streaming/Sync/Tap areas, and the four Space/OAuth TestServer hosts that overlap #136's DI redesign. 🤖 Generated with [Claude Code](https://claude.com/claude-code) 
Add tests/ATProtoNet.Tests/TestSupport/{HttpStub,XrpcTestClient}.cs: a
scripted HttpMessageHandler that keys routes by XRPC NSID or path,
records every request, and throws on an unscripted one instead of
silently answering. Fold the Bsky-only ScriptedXrpcHandler into it and
migrate its ten consumers.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace the per-file MockHttpMessageHandler in ChatActorClientTests,
ChatNotificationClientTests, ChatModerationClientTests,
GroupClientTests and ConvoClientTests with the shared HttpStub.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
HttpStub now locks around request recording and route dequeuing so
concurrent calls through the same stub (single-flight session refresh,
parallel admin calls) record and answer correctly.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add HttpStub.Fallback (answers any route with nothing scripted,
for cross-cutting tests that hit many endpoints without caring which),
HttpStub.Delay (for timeout/cancellation tests) and a typed UserAgent
reader (TryGetValues splits User-Agent into per-token values).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fix HttpStub to read a request's ContentType/ContentLength before
buffering its body: HttpContent.Headers.ContentLength can turn from
null (a non-seekable stream sent chunked) into the buffered byte count
once the content has been read, which broke a chunked-upload assertion.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
RequiresPdsFactAttribute, RequiresBlueskyFactAttribute,
RequiresPdsAdminFactAttribute, RequiresJetstreamFactAttribute,
RequiresJetstreamArchiveFactAttribute and RequiresSpacesFactAttribute
are replaced by [RequiresFact(IntegrationRequirement.X)], with a
[RequiresTheory(...)] counterpart. RequiresPdsFact and RequiresBlueskyFact
used to ignore ATPROTO_REQUIRE_INTEGRATION and always skip when their
prerequisite was missing; the unified attribute honours it like the
others, so a CI job whose environment silently stopped providing a
prerequisite fails instead of passing on skipped tests.

RequiresRedisFactAttribute is left alone (#136 removes the Redis store).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CreateDiscovery() previously built its IdentityResolver via
IdentityResolver.CreateDefault, which owns its own real HttpClients for
DID and handle resolution, entirely separate from the scripted handler
passed to AuthorizationServerDiscovery. If the SSRF pre-check these
tests exercise ever regressed, a refused identifier (169.254.169.254,
10.0.0.1, ...) could reach a real socket instead of the test's stub.
Wire the DID/handle resolvers through the same scripted handler
instead, using the existing HttpClient-accepting constructors.

Also replace the two bare `new HttpClient()` instances in the
SetServiceUrl tests (which never send a request) with a handler that
throws if one ever is, so a regression fails loudly instead of quietly
gaining the ability to dial out.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
DeleteAsync had no unit test through the RecordCollection<T> wrapper
(only via RepoClient.DeleteRecordAsync directly); add the happy path
and the swapRecord CAS parameter.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
XrpcTestHost and CapturingLoggerProvider lived in XrpcEndpointTests.cs
and were reused by four other files (ServiceAuthSchemeTests,
QueryLabelsEndpointTests, XrpcQueryBindingTests, XrpcRouteGroupTests,
ServiceAuthWithSpacesTests) only because they sit in a descendant
namespace — an implicit, easy-to-break sharing mechanism. Move both
into tests/ATProtoNet.Tests/TestSupport/ alongside HttpStub and
XrpcTestClient, so the host is found by an explicit using like every
other shared fixture, and add a mapXrpcEndpoints flag so a host that
maps only its own endpoints (no IXrpcQuery/IXrpcProcedure registered)
can opt out of MapXrpcEndpoints, which throws without one.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
An audit against the existing blanket coverage (Identity/IdentifierContractTests,
the Lexicon drift/JSON-name tests, and the pinned interop fixtures for
DAG-CBOR/CID/crypto) found tests that assert nothing beyond what that
coverage or a sibling test already pins:

- DTO getter/setter (6): OAuthException/JsonWebKey/JsonWebKeySet
  property echoes, two AtProtoRecord property-echo tests.
- Constant-equals-literal (1): RecordKey.Self, already asserted by
  IdentifierContractTests.ExplicitConversionsFromString_ParseOrThrow.
- Smoke tests asserting nothing meaningful (5): sub-client NotNull
  checks on AtProtoClient/Chat/Ozone/StandardSite, an OAuth client's
  default ServiceUrl/Session.
- Self-consistency round-trips subsumed by pinned vectors (16):
  five IdentitySerializationTests *_RoundTrips (each type already has
  a *_Serializes_AsString pinning the wire string), two DagCbor
  round-trip/CID-consistency tests duplicating DagCborEncoderTests and
  the pinned DagCborInteropTests fixture, MstNodeData determinism
  (pinned by the reference vectors), two CidComputation determinism/
  format checks (pinned by CidTests), and six CompareTo/explicit-cast
  duplicates of IdentifierContractTests' shared AssertContract.

30 test methods removed (one whole file, ChatClientsTests.cs); unit
test count goes from 4993 to 4963, all still green. No regression
tests, no security tests. A merge-duplicate-facts-into-theories pass
was scoped out for now — noted in the PR description as a further
opportunity, not applied here to keep this change reviewable.

Also fix two stale <see cref> doc comments left over from the
RequiresFact consolidation (JetstreamArchiveTests, JetstreamV2Tests).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
test: fix the rebase onto the builder PR (#137)
All checks were successful
CI / pds-integration (pull_request) Successful in 30s
Details
CI / build-and-test (pull_request) Successful in 58s
Details
Sync Closures to GitHub / sync-closure (pull_request) Successful in 7s
Details
5c05686193 
AtProtoBuilderTests (#174) used CapturingLoggerProvider from the server
test namespace; it now lives in TestSupport. The Redis replay-store tests
and [RequiresRedisFact] were removed by #174, so the CLAUDE.md lines this
branch had reworded for them go too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
opencode-bot commented 2026-09-26 22:11:44 +00:00
Collaborator
Copy link

🤖 Reviewing this PR — the review will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the review runs.

🤖 Reviewing this PR — the review will link the full session transcript. <!-- opencode-run:review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-175/run-2026-09-26T22-11-44-088Z.md) — updating every ~5 minutes while the review runs.
opencode-bot commented 2026-09-26 22:11:46 +00:00
Collaborator
Copy link

⏳ Held on the OpenCode Go window — the 5-hour window is at 0% (the bot holds at 95%).
This continues automatically after the window resets (2026-09-27T03:07:10.725Z) — nothing to re-trigger.

⏳ **Held on the OpenCode Go window** — the 5-hour window is at 0% (the bot holds at 95%). This continues automatically after the window resets (2026-09-27T03:07:10.725Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f2039c5b6ffejjtt6tmeEyh9p4 -->
Grandiras commented 2026-09-26 22:11:51 +00:00
Author
Owner
Copy link

@claude /review — test-only apart from the CONTRIBUTING, CLAUDE.md and testing-docs updates for the new attribute. Please check:
(1) Removed tests. For each one listed in the description, confirm that what it covered is still covered, and that none was a regression or security test. Use git diff origin/main...HEAD --diff-filter=D plus the removed methods in modified files.
(2) The migration to TestSupport/HttpStub.cs/XrpcTestClient.cs. Does any assertion get weaker? That includes request-body or header assertions dropped, and a stub that now accepts requests the old handler would have rejected.
(3) The SecurityTests SSRF rewiring. Does it still exercise the real guard (IdentityFetch/the connect-callback policy) rather than a stub that bypasses it?
(4) RequiresFact/IntegrationRequirement. Check that ATPROTO_REQUIRE_INTEGRATION turns a missing prerequisite into a failure for every requirement.

@claude /review — test-only apart from the CONTRIBUTING, CLAUDE.md and testing-docs updates for the new attribute. Please check: (1) **Removed tests.** For each one listed in the description, confirm that what it covered is still covered, and that none was a regression or security test. Use `git diff origin/main...HEAD --diff-filter=D` plus the removed methods in modified files. (2) **The migration to `TestSupport/HttpStub.cs`/`XrpcTestClient.cs`.** Does any assertion get weaker? That includes request-body or header assertions dropped, and a stub that now accepts requests the old handler would have rejected. (3) **The `SecurityTests` SSRF rewiring.** Does it still exercise the real guard (`IdentityFetch`/the connect-callback policy) rather than a stub that bypasses it? (4) **`RequiresFact`/`IntegrationRequirement`.** Check that `ATPROTO_REQUIRE_INTEGRATION` turns a missing prerequisite into a failure for every requirement.
claude-bot commented 2026-09-26 22:11:57 +00:00
Collaborator
Copy link

🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 Reviewing this PR — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/627); the reply will link the full session transcript. <!-- claude-run:pr_review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-175/run-627.md) — updating every ~5 minutes while the task runs.
claude-reviewer approved these changes 2026-09-26 22:14:55 +00:00
claude-reviewer left a comment
Copy link

✅ Verdict: approve — no blocking issues found.

This is a large but mechanical test-infrastructure refactor, and it holds up well under verification. I spot-checked the key claims against the actual branch content (not just the diff):

  • The HttpStub implementation is solid: thread-safe under one lock, throws loudly on an unscripted route (no silent-default footgun), correctly reads ContentType/ContentLength before buffering the body, and the repeat-last-queued-response semantics (queue.Count > 1 ? Dequeue() : Peek()) match the documented behavior and are exercised correctly by the migrated concurrent-call tests (e.g. ConcurrentAdminCalls_SignInOnlyOnce).
  • The IntegrationRequirement enum consolidation in TestInfrastructure.cs preserves the exact prior skip-reason text and gating logic per requirement, and correctly generalizes ATPROTO_REQUIRE_INTEGRATION to also cover Pds/Bluesky (which previously ignored it) — matches the PR's stated behavior change.
  • Checked the SecurityTests.cs SSRF rewiring: the CachingDidResolver/DidResolver/PlcClient/DidWebResolver/HandleResolver/IdentityResolver constructor overloads it now uses all exist with matching signatures in src/ATProtoNet/Identity/* — the PR's claim that "no src/ change needed, the seam already existed" checks out.
  • Confirmed via git grep that no leftover references to the old RequiresPdsFactAttribute/RequiresBlueskyFactAttribute/etc. remain in code (only historical CHANGELOG entries for already-released versions, which correctly stay as-is), and confirmed tests/ATProtoNet.Tests/Chat/ChatClientsTests.cs was actually deleted as claimed.
  • The list of hand-rolled handlers left unmigrated (Ozone, Server/Spaces/, Streaming/Jetstream, Tap, Spaces/SpaceClientTests, AtProtoOAuthServiceHttpClientTests) matches what's actually still in the tree.

One very minor nit noted inline: RequiresTheoryAttribute is added with no current callers. Not a blocker — it's cheap, documented as the Fact counterpart, and plausibly there to complete the pairing, but it is currently dead code by strict repo convention ("don't add features beyond what the task requires").

Overall this looks like careful, well-verified work — approving.

⏱ 2m 42s · 28 turns · tokens: 113.4k in / 9.2k out (+3286.5k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is a large but mechanical test-infrastructure refactor, and it holds up well under verification. I spot-checked the key claims against the actual branch content (not just the diff): - The `HttpStub` implementation is solid: thread-safe under one lock, throws loudly on an unscripted route (no silent-default footgun), correctly reads `ContentType`/`ContentLength` before buffering the body, and the repeat-last-queued-response semantics (`queue.Count > 1 ? Dequeue() : Peek()`) match the documented behavior and are exercised correctly by the migrated concurrent-call tests (e.g. `ConcurrentAdminCalls_SignInOnlyOnce`). - The `IntegrationRequirement` enum consolidation in `TestInfrastructure.cs` preserves the exact prior skip-reason text and gating logic per requirement, and correctly generalizes `ATPROTO_REQUIRE_INTEGRATION` to also cover Pds/Bluesky (which previously ignored it) — matches the PR's stated behavior change. - Checked the `SecurityTests.cs` SSRF rewiring: the `CachingDidResolver`/`DidResolver`/`PlcClient`/`DidWebResolver`/`HandleResolver`/`IdentityResolver` constructor overloads it now uses all exist with matching signatures in `src/ATProtoNet/Identity/*` — the PR's claim that "no src/ change needed, the seam already existed" checks out. - Confirmed via `git grep` that no leftover references to the old `RequiresPdsFactAttribute`/`RequiresBlueskyFactAttribute`/etc. remain in code (only historical CHANGELOG entries for already-released versions, which correctly stay as-is), and confirmed `tests/ATProtoNet.Tests/Chat/ChatClientsTests.cs` was actually deleted as claimed. - The list of hand-rolled handlers left unmigrated (Ozone, Server/Spaces/*, Streaming/Jetstream*, Tap, Spaces/SpaceClientTests, AtProtoOAuthServiceHttpClientTests) matches what's actually still in the tree. One very minor nit noted inline: `RequiresTheoryAttribute` is added with no current callers. Not a blocker — it's cheap, documented as the Fact counterpart, and plausibly there to complete the pairing, but it is currently dead code by strict repo convention ("don't add features beyond what the task requires"). Overall this looks like careful, well-verified work — approving. --- ⏱ 2m 42s · 28 turns · tokens: 113.4k in / 9.2k out (+3286.5k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-175/run-627.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:5c05686193355ce4a9c32c592c04f336c736db5d -->
tests/ATProtoNet.IntegrationTests/TestInfrastructure.cs
@ -170,2 +145,2 @@
"ATPROTO_PDS_ADMIN_PASSWORD environment variable.";
}
/// <summary>The <see cref="TheoryAttribute"/> counterpart of <see cref="RequiresFactAttribute"/>.</summary>
public sealed class RequiresTheoryAttribute : TheoryAttribute
claude-reviewer commented 2026-09-26 22:14:55 +00:00
Collaborator
Copy link

RequiresTheoryAttribute has no callers anywhere in the tree (no [RequiresXTheory] existed before this PR either, so nothing is being preserved). Minor, but per the repo's own "don't add features beyond what the task requires" convention, consider dropping it until a theory actually needs it, or note why it's added speculatively.

`RequiresTheoryAttribute` has no callers anywhere in the tree (no `[RequiresXTheory]` existed before this PR either, so nothing is being preserved). Minor, but per the repo's own "don't add features beyond what the task requires" convention, consider dropping it until a theory actually needs it, or note why it's added speculatively.
claude-bot commented 2026-09-26 22:14:55 +00:00
Collaborator
Copy link

🔧 Let Claude fix this — tick the box and claude-bot picks up the review above, on this PR branch. Nothing runs until you do.

  • @claude-bot Address the findings of the latest review on this pull request.
🔧 **Let Claude fix this** — tick the box and claude-bot picks up the review above, on this PR branch. Nothing runs until you do. - [ ] @claude-bot Address the findings of the latest review on this pull request. <!-- claude-fix-button:175 -->
Grandiras deleted branch issue-137-tests 2026-09-26 22:15:27 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
claude-reviewer
Labels
Clear labels   
breaking-change
Introduces a breaking API change

  
bug
Something is not working correctly

  
documentation
Documentation improvements

  
duplicate
This issue or PR already exists

  
enhancement
New feature or improvement

  
good first issue
Good for newcomers to the project

  
help wanted
Extra attention is needed

  
performance
Performance-related issue or improvement

  
question
Further information is requested

  
wontfix
This will not be worked on

No labels
breaking-change
bug
documentation
duplicate
enhancement
good first issue
help wanted
performance
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Grandiras/ATProto.NET!175
No description provided.