Skip to content

JustDummies: Any.Reproducibly's async overload lets a failing async test body pass green #317

Description

@Reefact

Problem

The library's flagship promise is that a broken test fails. But an async test body passed to Any.Reproducibly can make a failing test pass green — the worst possible outcome for a test-support tool.

Any.Reproducibly overloads sync and async on one name (Any.cs:511-571):

public static void Reproducibly(Action body, Action<string>? report = null)
public static void Reproducibly(int seed, Action body, Action<string>? report = null)
public static Task Reproducibly(Func<Task> body, Action<string>? report = null)          // async
public static async Task Reproducibly(int seed, Func<Task> body, Action<string>? report = null)  // async

The async overloads are internally correct (they await the body and report the seed). The defect is that they are too easy to reach by accident and drop:

[Fact]
public void Prices_are_positive() {
    Any.Reproducibly(async () => {              // async lambda → binds to Reproducibly(Func<Task>)
        var price = Any.Decimal().Generate();
        await _repository.SaveAsync(price);
        Assert.True(price > 0);                 // fails
    });
    // ⚠️ this expression returns a Task that nobody awaits
}

An async lambda is a better conversion to Func<Task> than to Action, so it binds to the async overload, which returns a Task. The test method is void and does not await it, so the Task is discarded. The assertion runs on a continuation after the method has already returned normally → the test is green; the failure becomes an at-best-later UnobservedTaskException. The same happens for Reproducibly(() => SomeAsyncMethod()) (an expression lambda whose body is a Task).

Two design facts make this sharp:

  • The async overload is named Reproducibly and returns Task — a TAP naming-convention violation (Task-returning ⇒ Async suffix), and a returned Task is trivially droppable.
  • The compiler's own CS4014 ("this call is not awaited") does not fire here, because the enclosing test method is a synchronous void.
  • Naively removing the Func<Task> overload does not fix it: an async lambda would then bind to Reproducibly(Action) as async void, whose post-await exception escapes the try/catch entirely — strictly worse. The current Func<Task> overload is paradoxically what prevents async void; its only flaw is the droppable Task.

Why this must be fixed before 1.0

  • A silent green is the worst failure mode for a test tool; it quietly defeats the library's entire reason to exist.
  • The overload set and method names are frozen public API at 1.0. Renaming or changing the return shape afterwards is source-breaking, so this is a pre-tag decision.

Decision (from the design discussion)

Close both failure modes without shipping any deprecated member and without blocking:

  • Rename the two async overloads to ReproduciblyAsync (TAP-correct, returns Task, awaited by the caller).
  • No [Obsolete] "poison" overload. A member deprecated in a brand-new 1.0 is a contradiction and clutters the shipped surface.
  • No blocking (GetAwaiter().GetResult()) overload. Sync-over-async risks deadlock under a captured SynchronizationContext — the anti-pattern async exists to avoid.
  • Introduce a JustDummies analyzer. FirstClassErrors already ships analyzers (FCE001FCE022); JustDummies gains its own, error-agnostic analyzer with two error-severity rules:
    • JD001 — an async lambda (or any lambda whose body is a Task) passed to Reproducibly(Action)"Use ReproduciblyAsync and await it." Closes the async void hazard at compile time.
    • JD002 — a call to ReproduciblyAsync(...) whose returned Task is discarded (not awaited, assigned, or returned) → error. Closes the dropped-Task hazard that CS4014 misses in synchronous test methods.

The language cannot express either rule (there is no non-droppable Task, and no way to forbid an async-lambda→Action conversion), so an analyzer is the legitimate tool here (ADR-0035: types where they can carry the rule, analyzers for what they cannot).

Scope

  1. Rename Reproducibly(Func<Task>) / Reproducibly(int, Func<Task>)ReproduciblyAsync. Update the internal call site (SeedReproducibilityTests.cs), the public API baselines (netstandard2.0 + net8.0), the XML docs, and the user guide (EN + FR). The xUnit [Reproducible] adapter is not affected — it drives Any.UseSeed(...) directly, not the async Reproducibly.
  2. New JustDummies.Analyzers project (netstandard2.0, Roslyn, no dependency on FirstClassErrors — the JustDummies error-agnostic boundary must stay intact) + JustDummies.Analyzers.UnitTests. Wire both GUIDs into FirstClassErrors.sln's GlobalSection(NestedProjects) (under src / tests). Package the analyzer into the JustDummies NuGet at analyzers/dotnet/cs, mirroring the _AddAnalyzerToPackage pattern in FirstClassErrors.csproj (taking care not to add the DLL twice across the two target frameworks). Own diagnostic-ID scheme (JD0xx).
  3. JD001 + JD002 implemented with analyzer unit tests, including a documented help-link per diagnostic if the analyzer scheme uses one.
  4. Tests first. Demonstrate the current silent-green empirically (a failing async body under the old overload passes), then confirm the two diagnostics go red on the offending code and green on the correct code (Reproducibly(() => {...}) sync, await ReproduciblyAsync(async () => {...})).
  5. ADR (Proposed) — "JustDummies ships analyzers": a new cross-cutting capability plus a JustDummies diagnostic-ID policy, and the async-safety contract (ReproduciblyAsync + JD001/JD002) it establishes.

Acceptance criteria

  • A failing async test body can no longer pass green: passing an async lambda to Reproducibly is a compile error (JD001) that names ReproduciblyAsync; discarding a ReproduciblyAsync result is a compile error (JD002).
  • await ReproduciblyAsync(...) propagates the failure and reports the seed exactly as the async Reproducibly did before — behaviour unchanged, name changed.
  • Reproducibly(Action) (sync) is untouched.
  • The analyzer ships inside the JustDummies package and carries no dependency on FirstClassErrors; the JustDummies.UnitTests architecture-boundary test stays green.
  • The public API baseline is updated deliberately (the rename is a breaking change; acceptable pre-1.0, no consumers). Solution build stays at 0 warnings; analyzer tests are green.
  • Diagnostic IDs, messages, documentation, and tests are mutually consistent.

Context

Surfaced by the 2026-07 JustDummies v1.0.0 readiness audit as problem #3 (BLOCKER / MUST FIX): the library can silently pass a broken async test. Sibling of the concurrency fix (#310 / #311) and the conflict-message fix (#312 / #313). The design discussion explicitly rejected an [Obsolete] poison overload (a deprecated member in a fresh 1.0) and a blocking sync-over-async overload (deadlock risk) in favour of a rename plus a first-party analyzer.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions