Redesign mixed tests template

[thomhurst]

Summary

• Replaces the existing mixed tests template with a clean, well-organized structure built around a Calculator SUT and InMemoryDb fixture
• Tests grouped by feature: BasicTests, DataDrivenTests, DependencyInjectionTests, HooksAndLifecycle
• Removes verbose Console.WriteLine commentary — code speaks for itself
• Drops IClassConstructor example and [assembly: Retry(3)] (bad default to teach)
• Targets net10.0, pins TUnit version to 1.*

What the template covers

• BasicTests — simple test, exception assertion, [Before(Class)] and [Before(Test)] hooks
• DataDrivenTests — [Arguments], [MethodDataSource], custom [DataGenerator], [MatrixDataSource]
• DependencyInjectionTests — [ClassDataSource] (new per test vs shared), class-level injection, property injection
• HooksAndLifecycle — [Before/After(TestSession)], [After(Class)], [After(Test)], [assembly: ExcludeFromCodeCoverage]
• InMemoryDb — IAsyncInitializer / IAsyncDisposable lifecycle
• AdditionDataGenerator — custom DataSourceGeneratorAttribute

Test plan

• Template builds successfully (dotnet build)
• All 23 tests pass (dotnet run)

[claude]

Code Review

This is a substantial improvement over the old template. The old version was cluttered with narration, a bad default, and the niche IClassConstructor pattern. The new template is cleaner, more representative, and built around a proper SUT. Good work overall.

---

Potential Bug: Non-static [After(Test)] in GlobalHooks

File: HooksAndLifecycle.cs

public class GlobalHooks
{
    [After(Test)]
    public Task AfterTest(TestContext context)  // instance method, no tests in this class
    {
        return Task.CompletedTask;
    }
}

For a global [After(Test)] hook to run after every test across all classes, it needs to be static. As an instance method in a class with no [Test] methods, this will not be invoked at all — the template silently demonstrates a no-op. The global [Before/After(TestSession)] and [After(Class)] hooks are correctly static, so this appears to be an oversight.

Fix: Make AfterTest static:

[After(Test)]
public static Task AfterTest(TestContext context)
{
    return Task.CompletedTask;
}

---

Tautological Test in DataDrivenTests

File: DataDrivenTests.cs

[MatrixDataSource]
public async Task Multiply_AllCombinations(
    [Matrix(1, 2, 3)] int a,
    [Matrix(0, 1, -1)] int b)
{
    var result = calculator.Multiply(a, b);
    await Assert.That(result).IsEqualTo(a * b);  // asserts a * b == a * b
}

The assertion IsEqualTo(a * b) is tautological — it compares the result of Calculator.Multiply against the runtime's own * operator, so this test can never fail (unless the CLR itself has a bug). It doesn't demonstrate what matrix testing is actually useful for.

Better approach: Use a pre-computed expected value that the reader can verify at a glance. For MatrixDataSource, the point is combinations — show it asserting something meaningful:

[Test]
[MatrixDataSource]
public async Task Add_WithMatrix(
    [Matrix(1, 2)] int a,
    [Matrix(10, 20)] int b)
{
    var expected = a + b; // clearly derivable, or use a lookup
    var result = calculator.Add(a, b);
    await Assert.That(result).IsEqualTo(expected);
}

Or even better, use a different operation where the expected values are hardcoded and the test would catch a real regression.

---

Redundant Initialization in InMemoryDb

File: Data/InMemoryDb.cs

private Dictionary<string, string> _store = new();  // initialized here

public Task InitializeAsync()
{
    _store = new Dictionary<string, string>();  // redundant re-initialization
    return Task.CompletedTask;
}

The field initializer makes the InitializeAsync assignment redundant. While harmless, newcomers reading this template may wonder why _store is initialized twice. The field initializer = new() can simply be dropped since InitializeAsync always runs before the object is used (per the IAsyncInitializer contract). Or the field initializer can be removed and the comment updated to explain that InitializeAsync is the right place for setup.

---

Suggestion: Demonstrate Class-Level [Arguments]

The old Tests2.cs demonstrated class-level [Arguments] (parameterising the constructor), which is a distinctive TUnit feature not available in most testing frameworks:

[Arguments("Hello")]
[Arguments("World")]
public class MoreTests(string title) { ... }

The new template drops this entirely. It's worth adding a small example in DataDrivenTests.cs or DependencyInjectionTests.cs since class-level parameterization is one of TUnit's standout capabilities and helps users discover it early.

---

Minor: Version="1.*" in TestProject.csproj

Wildcard versions can cause non-deterministic restores since NuGet always resolves to the latest matching version. For a project template, this means two users running dotnet new tunit at different times may get different package versions, which can cause unexpected breakage. A floating patch version like 1.18.* or even leaving it to the template metadata to inject the version might be worth considering.

---

Summary

Issue	Severity
Non-static [After(Test)] in GlobalHooks (silent no-op)	High
Tautological Multiply_AllCombinations assertion	Medium
Redundant _store initialization	Low
Missing class-level [Arguments] example	Suggestion
Wildcard NuGet version	Low

The overall direction of this PR is excellent — a real SUT, a realistic fixture, proper async patterns, and no assembly-level retry defaults. The structural reorganization by feature makes the template significantly more navigable.

[claude]

Code Review

This is a substantial improvement over the old template. The old version was cluttered with Console.WriteLine narration, a bad assembly-level Retry(3) default, and the niche IClassConstructor pattern. The new template is cleaner, more representative, and built around a proper SUT. Good work overall.

---

Potential Bug: Non-static [After(Test)] in GlobalHooks

File: HooksAndLifecycle.cs

The AfterTest method in GlobalHooks is an instance method:

public class GlobalHooks
{
    [After(Test)]
    public Task AfterTest(TestContext context)  // instance method, no tests in this class
    {
        return Task.CompletedTask;
    }
}

For a global [After(Test)] hook to run after every test across all classes, it needs to be static. As an instance method in a class with no [Test] methods, this will not be invoked at all — the template silently demonstrates a no-op. The sibling [Before/After(TestSession)] and [After(Class)] hooks in the same class are correctly static, so this appears to be an oversight.

Fix:

[After(Test)]
public static Task AfterTest(TestContext context)
{
    return Task.CompletedTask;
}

---

Tautological Test in DataDrivenTests

File: DataDrivenTests.cs

var result = calculator.Multiply(a, b);
await Assert.That(result).IsEqualTo(a * b);  // asserts a * b == a * b

The assertion IsEqualTo(a * b) is tautological — it compares Calculator.Multiply against the runtime's own * operator, so this test can never fail unless the CLR itself is broken. It doesn't demonstrate what matrix testing is useful for, and it's a confusing pattern for newcomers to copy.

Better approach: Verify a property such as commutativity using a different code path, or use hardcoded expected values:

// Verify commutativity via symmetry
await Assert.That(calculator.Multiply(a, b)).IsEqualTo(calculator.Multiply(b, a));

---

Redundant Initialization in InMemoryDb

File: Data/InMemoryDb.cs

private Dictionary<string, string> _store = new();  // initialized here

public Task InitializeAsync()
{
    _store = new Dictionary<string, string>();  // redundant re-assignment
    return Task.CompletedTask;
}

The field initializer makes the InitializeAsync assignment redundant. While harmless, newcomers may wonder why _store is initialized twice. The field initializer can be dropped (since InitializeAsync runs before the object is used per the IAsyncInitializer contract), or a comment can clarify the intent.

---

Suggestion: Demonstrate Class-Level [Arguments]

The old Tests2.cs demonstrated class-level [Arguments] (parameterising the constructor), which is a distinctive TUnit feature not available in most testing frameworks. The new template drops this entirely. It is a standout TUnit capability and worth keeping so new users discover it early. A small example could be added to DataDrivenTests.cs.

---

Minor: Version="1.*" in TestProject.csproj

Wildcard versions cause non-deterministic restores — two users running dotnet new tunit at different times may get different package versions, which can cause unexpected breakage. A version range like [1.0,2.0) or a specific version pinned at template generation time would be more reproducible. If staying with a wildcard, at least 1.* is bounded by major version so it is not the worst option.

---

Summary

Issue	Severity
Non-static [After(Test)] in GlobalHooks (likely a silent no-op)	High
Tautological Multiply_AllCombinations assertion	Medium
Redundant _store initialization in InMemoryDb	Low
Missing class-level [Arguments] example	Suggestion
Wildcard NuGet version	Low

The overall direction is excellent — a real SUT, a realistic fixture, proper async patterns, and no harmful assembly-level defaults. The reorganization by feature area makes the template much more navigable for new users.

[claude]

Follow-up Review

Good progress since the previous review — most of the feedback has been addressed. Here's the status.

---

✅ Issues Resolved

Non-static [After(Test)] in GlobalHooks (previously High severity)
Resolved cleanly by separating concerns: BasicTests.cs now correctly uses instance [Before(Test)]/[After(Test)] for class-scoped hooks (which is correct — instance hooks scope to the containing test class), while HooksAndLifecycle.cs keeps only TestSession-level static hooks. This is the right design.

Redundant _store initialization in InMemoryDb
Fixed with private Dictionary<string, string> _store = null\!. The single initialisation in InitializeAsync now clearly signals the IAsyncInitializer lifecycle intent.

Missing class-level [Arguments] example
Added as ClassLevelArgumentTests in DataDrivenTests.cs. Good addition.

---

❌ Still Outstanding: Tautological Multiply_AllCombinations Assertion

// DataDrivenTests.cs
var result = calculator.Multiply(a, b);
await Assert.That(result).IsEqualTo(a * b);  // asserts a * b == a * b

This test cannot fail unless the CLR's * operator is broken — Calculator.Multiply does a * b, and the assertion verifies a * b. This was flagged in the previous review and remains unchanged.

For a template meant to teach TUnit to new users, this is worth fixing because it models a bad testing habit (tautological assertions). The matrix test is a great feature to showcase — it just needs a meaningful expected value.

Suggested approach: Either use hardcoded expected values derived from the matrix inputs, or demonstrate commutativity (a genuinely useful property to verify):

[Test]
[MatrixDataSource]
public async Task Multiply_IsCommutative(
    [Matrix(1, 2, 3)] int a,
    [Matrix(0, 1, -1)] int b)
{
    var calculator = new Calculator();

    // Demonstrates matrix expansion AND tests a real property
    await Assert.That(calculator.Multiply(a, b)).IsEqualTo(calculator.Multiply(b, a));
}

Or simply pick a non-trivial operation where hardcoded values make sense and the matrix expansion benefit is obvious.

---

Minor: Version="1.*" in TestProject.csproj

Still a floating version (flagged previously). Two users running dotnet new tunit at different times may resolve different packages and get different behaviour. This is low severity but worth considering whether the template tooling can inject the current version at generation time, or whether 1.* is an intentional "always latest 1.x" decision. If the latter, a comment in the .csproj would help template consumers understand the choice.

---

New Observation: ClassLevelArgumentTests.Divide_WithClassAndMethodArguments

[Arguments(10)]
[Arguments(100)]
public class ClassLevelArgumentTests(int divisor)
{
    [Test]
    [Arguments(100)]
    [Arguments(50)]
    public async Task Divide_WithClassAndMethodArguments(int dividend)
    {
        var result = calculator.Divide(dividend, divisor);
        await Assert.That(result).IsGreaterThan(0);
    }
}

With divisors {10, 100} and dividends {100, 50}, all four combinations produce positive results, so IsGreaterThan(0) will always pass. Consider making the assertion slightly more specific — e.g. IsEqualTo(10.0) for the (100 / 10) case (using expected values that are obvious to the reader). The goal is to show that the test could fail if the implementation was wrong, which is the whole point of parameterised tests.

---

Summary

Issue	Status
Non-static [After(Test)] in GlobalHooks	✅ Resolved
Redundant _store initialization	✅ Resolved
Missing class-level [Arguments] example	✅ Resolved
Tautological Multiply_AllCombinations	❌ Still present
ClassLevelArgumentTests weak assertion	New
Wildcard NuGet version	Low / carry-over

The template is significantly better than what it replaces. The main outstanding item is the tautological matrix test — fixing that would make this a solid, merge-ready template.