Skip to content

Add abstractions package boundary test#1197

Merged
mtmk merged 1 commit into
release/3.0from
verify-abstractions-boundary
Jun 25, 2026
Merged

Add abstractions package boundary test#1197
mtmk merged 1 commit into
release/3.0from
verify-abstractions-boundary

Conversation

@mtmk

@mtmk mtmk commented Jun 23, 2026

Copy link
Copy Markdown
Member

Adds a test project that references only NATS.Client.Abstractions (no Core) and exercises the use case behind the abstractions package: a third party such as a CloudEvents adapter can deconstruct a received message and build an outgoing one using just the envelope (NatsMsgContext / INatsHeaders) and the serializer interfaces. Runs in Linux and Windows CI.

Part of #851.

Add an Abstractions-only test project (references just NATS.Client.Abstractions,
no Core) covering the issue #851 use case: a third party such as a CloudEvents
adapter can deconstruct a received message and build an outgoing one using only
the envelope (NatsMsgContext / INatsHeaders) and the serializer interfaces. Wire
it into the Linux and Windows CI runs.
@mtmk mtmk added this to the Release 3.0 milestone Jun 23, 2026
@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mtmk's task in 3m 53s —— View job


PR Review: Add abstractions package boundary test

  • Read changed files and gather context
  • Review test project and boundary test logic
  • Check CI workflow changes
  • Post review findings

The PR is well-structured and correctly achieves its goal. The test project enforces the package boundary via .csproj — the only ProjectReference is to NATS.Client.Abstractions — which is the right place to put the guard. A few observations:

using NATS.Client.Core; at the top of the test file

This is correct but potentially surprising: all types in NATS.Client.Abstractions (e.g. INatsHeaders, NatsMsgContext, INatsSerialize*) live in the NATS.Client.Core namespace, even though they ship in the Abstractions package. The existing class-level comment (lines 9–11) goes some way toward explaining this, but the using statement directly contradicts the prose. Nudging the comment to also mention "the NATS.Client.Core namespace is exported by the Abstractions package" would make this less surprising to a future reader.

Context-aware serialize path is untested

CloudEventDeserializer.Serialize(IBufferWriter<byte>, CloudEventLike, in NatsMsgContext) (line 66) exists and is exercised only indirectly — the test (line 36) calls via the non-context INatsSerialize<T> overload. The INatsSerializeWithContext<T> dispatch in NatsSerializationExtensions.Serialize goes uncovered. Given the test's purpose is to prove the interface hierarchy compiles and resolves against Abstractions-only, this is acceptable — not calling it out as a bug, just an easy gap to fill.

Test correctness

The NatsSerializationExtensions.Deserialize extension method dispatch is correctly exercised: the test variable is typed as INatsDeserialize<CloudEventLike>, so deserializer.Deserialize(payload, context) resolves to the extension method, which internally casts to INatsDeserializeWithContext<T> and calls the context-overload. That's the exact behaviour the boundary test should exercise.

TestBufferWriter is correct for its purpose

The Ensure growth logic (Math.Max(_buffer.Length * 2, _written + sizeHint)) correctly handles zero-hint and large-write cases. The note about ArrayBufferWriter<byte> being unavailable on net481 is a good explanation for the hand-rolled version.

CI changes are clean

  • Both Linux and Windows steps correctly omit the nats-server kill/start since these tests need no broker.
  • --no-build is safe: the root-level dotnet build -c Release step covers the whole solution (the new project is added to NATS.Net.slnx).
  • net8.0;net10.0 + net481 on Windows and RuntimeIdentifiers with win-x86 all match the established pattern in sibling test projects (NATS.Client.CoreUnit.Tests, NATS.Client.Core2.Tests).

No blocking issues.

@mtmk
mtmk requested a review from scottf June 24, 2026 11:14
@mtmk mtmk added the ready label Jun 24, 2026

@scottf scottf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants