Skip to content

Add inbox generated tests#4230

Draft
lillo42 wants to merge 7 commits into
masterfrom
add.inbox.generated.tests
Draft

Add inbox generated tests#4230
lillo42 wants to merge 7 commits into
masterfrom
add.inbox.generated.tests

Conversation

@lillo42

@lillo42 lillo42 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Description

Related Issues

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the Contributing Guide
  • I have checked the documentation for relevant guidance
  • I have added/updated XML documentation for any public API changes
  • I have added/updated tests as appropriate
  • My changes follow the existing code style and conventions

Additional Notes

@lillo42 lillo42 added 2 - In Progress .NET Pull requests that update .net code V10.X labels Jul 12, 2026
@claude

claude Bot commented Jul 12, 2026

Copy link
Copy Markdown

Code Review — PR #4230: Add inbox generated tests

Nice addition. This extends the established test-generator pattern (OutboxGenerator/MessagingGatewayGenerator) to the inbox, wiring up InboxConfiguration + InboxGenerator, the Liquid templates, and per-provider test-configuration.json entries for MySQL, PostgreSQL, and SQLite. I verified the plumbing end-to-end and it's largely solid. A few things worth addressing:

🟠 Missing license headers on generated test templates

14 of the 16 inbox templates (every When_* scenario, both Sync and Async) start directly with // <auto-generated> and omit the #region Licence … #endregion MIT block. Only the two interface templates (IAmAnInboxProviderSync/Async) include it.

This is inconsistent with the outbox generated tests — e.g. tests/Paramore.Brighter.Sqlite.Tests/Outbox/Text/Generated/Sync/When_Adding_A_Duplicate_Message_It_Should_Not_Throw.cs — which all carry the header, and with the repo's documentation standard requiring license headers. Since these are regenerated from templates, the fix belongs in the .liquid files (then regenerate).

🟠 MySQL async provider disposes connections synchronously

In tests/Paramore.Brighter.MySQL.Tests/Inbox/Text/MySqlInboxProviderBase.cs, CreateStoreAsync/DeleteStoreAsync use using var connection/using var command while awaiting the actual work:

```csharp
public async Task CreateStoreAsync()
{
using var connection = new MySqlConnection(Configuration.ConnectionString); // sync dispose
await connection.OpenAsync();
...
}
```

MySqlConnection is IAsyncDisposable, and the SQLite/Postgres bases here correctly use await using. For consistency and to avoid a synchronous dispose on an async-used connection, these should be await using var.

🟡 Base-class namespace smell

SqliteInboxProviderBase, MySqlInboxProviderBase, and PostgresInboxProviderBase are declared in the ...Inbox.Text namespace, but they're the shared base for the Binary/Json/Jsonb providers too. That forces unrelated variants to using ...Inbox.Text; (see PostgresJsonInboxProvider.cs, SqliteBinaryInboxProvider.cs). The base isn't Text-specific — placing it in ...Inbox would read more naturally. (The outbox equivalents don't share a base class, so this is a new convention introduced here.)

🟡 XML doc cref targets a non-existent generic type

The interface templates document the factory method as returning <see cref="IAmAnInboxSync{T}"/> / <see cref="IAmAnInboxAsync{T}"/>, but those interfaces are non-generic (IAmAnInboxSync / IAmAnInboxAsync). The cref won't resolve — drop the {T}.

✅ Verified and looking good

  • DDL flag mapping is correct for every Postgres variant: Text base(false,false), Binary base(true,false), Json base(false,true), Jsonb base(true,true) — matches PostgreSqlInboxBuilder.GetDDL(binary, json) branching.
  • Generated calls match the core interface signatures (Add<T>, Get<T>, Exists<T>, and async variants with the contextKey/requestContext params).
  • Async tests correctly use IAsyncLifetime (InitializeAsync/DisposeAsync); sync tests use IDisposable.
  • Unique per-test table names ({TablePrefix}{Uuid:N}) avoid cross-test collisions; SQLite enables WAL.
  • .csproj updated to copy the new Templates/Inbox/** folders to output; Program.cs invokes the new generator.
  • Good scenario coverage: add/retrieve, duplicate (same & different context key), exists true/false, and not-found / wrong-context-key exceptions — across sync + async and all storage variants.

None of these are blocking; the two 🟠 items (license headers, async dispose) are the ones I'd fix before merge.

🤖 Automated review by Claude Code

@claude

claude Bot commented Jul 12, 2026

Copy link
Copy Markdown

PR Review: Add inbox generated tests

Thanks for this — it's a clean, consistent migration of the inbox integration tests from inheritance-based shared base classes (RelationalDatabaseInboxTests etc.) to Liquid-template–generated tests, matching the pattern already used for the outbox and messaging-gateway generators. I focused on the generator infrastructure and templates (the source of truth) plus a sampling of the generated output and hand-written providers.

What looks good

  • Coverage is preserved and expanded. The old base classes covered 6 scenarios; the templates reproduce all 6 and add When_Checking_If_An_Existing_Command_Exists_It_Should_Return_True (sync + async). Net gain.
  • API usage verified. Add/Get<T>/Exists<T> calls in the templates match IAmAnInboxSync/IAmAnInboxAsync signatures.
  • Provider abstraction is tidy. The generated IAmAnInboxProviderSync/Async interfaces + hand-written providers cleanly separate store lifecycle from the inbox-under-test, and table/collection names are per-instance GUIDs (Uuid.New():N) so tests are isolated even before collection serialization.
  • Committed generated output matches the templates (spot-checked the PostgreSQL Binary sync file), license headers and XML docs are consistent, and the retained base classes are still used by GCP/Firestore/Spanner/Core tests (not orphaned).

Suggestions / minor issues

  1. Invalid XML cref in the provider-interface templates. In Templates/Inbox/Sync/IAmAnInboxProviderSync.cs.liquid and the async variant, the <returns> docs reference <see cref="IAmAnInboxSync{T}"/> / <see cref="IAmAnInboxAsync{T}"/>. Those interfaces are non-generic (public interface IAmAnInboxSync : IAmAnInbox), so the cref won't resolve (CS1574 if doc warnings are on). Drop the {T}. This is baked into every generated copy.

  2. Provider-base namespace placement. PostgresInboxProviderBase, MySqlInboxProviderBase, and MSSQLInboxProviderBase live in the ...Inbox.Text namespace but serve as the base for the Binary/Json/Jsonb providers too. It compiles, but a neutral namespace (e.g. ...Inbox) would read better.

  3. Cosmetic blank line in generated attributes. The {% if Category %} / {% if CollectionName %} conditionals leave a blank line between [Trait(...)] and [Collection(...)] in the output. Harmless, but Liquid whitespace control ({%- ... -%}) would tidy it.

  4. Sync setup in the constructor can leak a store. The sync templates call _inboxProvider.CreateStore() in the constructor and drop it in Dispose(). If CreateStore() throws, xUnit does not call Dispose(), so the created table/collection can leak. The async variant avoids this via IAsyncLifetime. Minor and pre-existing in style, but worth considering.

  5. DynamoDB provider uses sync-over-async + shared static table. DynamoDBInboxProvider.CreateInbox()/CreateStore() call EnsureTableIsCreatedAsync(...).GetAwaiter().GetResult(), and the table name is static/shared (DynamoDbInboxTable.s_tableName). It's correct because all generated Dynamo tests share Collection("DynamoDBInbox") and are serialized, but it means the single physical table is dropped/recreated per test method (slow). Acceptable for test helpers — flagging for awareness.

  6. No drift guard for generated output. Generation is manual via generate-test.sh; the generated files are committed with no CI check that they still match the templates. Consider a CI step that re-runs the generator and fails on a non-empty git diff, so template edits can't silently diverge from committed tests.

None of these are blockers — (1) is the only one I'd fix before merge since it propagates to every generated file. Nice work consolidating this.

Reviewed with Claude Code.

@claude

claude Bot commented Jul 12, 2026

Copy link
Copy Markdown

Code Review — #4230 Add inbox generated tests

Thanks for this — migrating the inbox tests to the template/generator approach (matching the existing outbox & messaging-gateway generators) is a nice consistency win, and the bulk of the 338 files are mechanically generated .cs outputs plus their Liquid templates. I focused the review on the hand-written pieces: the generator infrastructure, the Liquid templates, the per-provider *InboxProvider classes, and the removed hand-written tests.

Overall this looks solid and the generated output faithfully reproduces the old suite. A few things worth addressing before merge.

🟡 1. CONNECTION_STRING env-var override is dropped for relational inbox tests

The removed RelationalDatabaseInboxTests base honored an environment override:
```csharp
var connectionString = Environment.GetEnvironmentVariable("CONNECTION_STRING");
if (string.IsNullOrWhiteSpace(connectionString)) connectionString = DefaultConnectingString;
```
The new provider bases (MSSQLInboxProviderBase, MySqlInboxProviderBase, PostgresInboxProviderBase, SqliteInboxProviderBase) hardcode Configuration.DefaultConnectingString / Const.ConnectionString and no longer read CONNECTION_STRING. The outbox and locking relational bases (RelationDatabaseOutboxTest, RelationalDatabaseDistributedLockingAsyncTest) still read it, so relational inbox tests are now inconsistent with the rest of the suite. CI doesn't currently set the variable, so this won't break the pipeline — but anyone running the suite against a non-default DB (custom container/port) will find inbox tests connect to the wrong server while outbox tests pass. Please restore the override in the provider bases (or drop it everywhere for consistency).

🟡 2. Orphaned base test classes left behind

Now that every hand-written inbox test was deleted, these four files in tests/Paramore.Brighter.Base.Test/Inbox/ have no remaining inheritors:

  • InboxTests.cs
  • InboxAsyncTest.cs
  • RelationDatabaseInboxTests.cs (RelationalDatabaseInboxTests)
  • RelationDatabaseInboxAsyncTests.cs

Verified: no : InboxTests / : RelationalDatabaseInboxTests (etc.) matches outside the files themselves. They should be deleted so the generated templates remain the single source of truth (otherwise the two definitions can drift).

🟢 3. Minor — MySQL async connection disposal

MySqlInboxProviderBase.CreateStoreAsync/DeleteStoreAsync use using var connection instead of await using, unlike the MSSQL and Postgres bases which correctly use await using. Prefer await using in the async paths for consistent async disposal.

🟢 4. Minor — template cosmetics / naming

  • The inbox When_*.liquid templates put [Trait]/[Collection] on their own lines with a leading newline inside the {% if %}, producing extra blank lines in the generated files (e.g. a blank line between [Trait] and [Collection]). The outbox templates keep them on one line — worth matching for cleaner output.
  • IAmAnInboxProviderAsync.CreateInboxAsync() returns IAmAnInboxAsync synchronously (not a Task); the Async suffix is a little misleading, though it's consistent with the sync provider shape.

✅ Things that look good

  • Coverage parity + a gain: all six original facts are reproduced, and the generated set adds When_Checking_If_An_Existing_Command_Exists_It_Should_Return_True, which the old base classes lacked.
  • Parallelism handled: splitting one test class into ~13 generated classes could have raced on shared stores (notably the static s_tableName in DynamoDbInboxTable, now with the new Reset()), but each provider sets CollectionName, so xUnit serializes the classes within a provider's collection. Good catch.
  • Nice reuse: the relational *InboxProviderBase + thin Text/Binary/Json subclasses passing binaryMessagePayload/jsonMessagePayload flags keeps the per-provider code minimal.

❓ Worth a sanity check

  • SQLite Text and Binary inboxes share the single test.db file but live in separate collections, so they run in parallel. Different table names should keep them independent, but concurrent CREATE/DROP TABLE on the same file can occasionally surface database is locked even with WAL. Keep an eye on flakiness there.

Nothing here is a blocker beyond #1 and #2. Nice work overall.

Reviewed against the repo's CLAUDE.md conventions (Change Scope, generated-tests workflow).

@iancooper

Copy link
Copy Markdown
Member

I have a change coming to Inbox that is sitting behind bug fixes, so we might want to wait for that in a few days

@lillo42

lillo42 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

I have a change coming to Inbox that is sitting behind bug fixes, so we might want to wait for that in a few days

No problem

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

Labels

2 - In Progress .NET Pull requests that update .net code V10.X

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants