Skip to content

Inject ConsoleReporter's output writers so its tests can run in parallel #328

Description

@rdeago

Reference version

2.1.84-preview (raised in the Claude review of PR #327)

Background and motivation

ConsoleReporter writes directly to Console.Out / Console.Error, so ConsoleReporterTests can only observe its output by swapping the process-global console writers (Console.SetOut / Console.SetError). That costs three things: the whole test class (currently ~20 tests) is [NotInParallel], a TUnit0055 suppression is needed because overwriting the console writers can break TUnit's own logging, and every new reporter test inherits the same ceremony through the CaptureConsole helper.

.claude/rules/testing.md says the preferred answer to hard-to-test plumbing is "a small, honest design change" that makes the code testable — not environment juggling. This is exactly that case, as the PR #327 review pointed out.

Proposed enhancement

ConsoleReporter tests construct reporters bound to private writers and run in parallel, with no process-global state, no [NotInParallel], and no analyzer suppression. bv's observable behavior does not change.

Implementation proposals

  • Add an internal constructor ConsoleReporter(Verbosity verbosity, bool? colorOverride, TextWriter output, TextWriter error); the existing public constructor delegates with Console.Out / Console.Error.
  • Semantics note: today every write reads Console.Error at write time, so a later SetError would be honored; with injected writers the binding happens at construction. Immaterial for bv (the reporter is constructed once in Program and nothing swaps the console afterwards), but the remarks should state it. Storing Func<TextWriter> accessors instead would preserve write-time binding, at the price of indirection nobody currently needs. (EDIT: although unneeded, this is a correctness issue in ConsoleReporter, so I'd lean on doing it. One day Buildvana.Core.ConsoleOutput might be promoted to its own package, so it's important that it don't assume bv as the only caller.)
  • Buildvana.Core.ConsoleOutput gains <InternalsVisibleTo Include="Buildvana.Core.ConsoleOutput.Tests" /> (same pattern as Buildvana.Tool).
  • ConsoleReporterTests drops [NotInParallel], the TUnit0055 suppression, and the writer-swapping CaptureConsole helper in favor of constructing with StringWriters.
  • The DetectColor / IsNoColorSet coverage exclusions stand either way: they read process-global console/environment state by design.

Usage examples

Not user-facing; the kind of test this enables:

using var stderr = new StringWriter();
var reporter = new ConsoleReporter(Verbosity.Normal, colorOverride: false, TextWriter.Null, stderr);
reporter.Error("boom");
await Assert.That(stderr.ToString()).IsEqualTo($"error: boom{Environment.NewLine}");

Risks

Test-design refactor with no behavior change; Buildvana.Core.* is unpackaged, so no public surface moves and no changelog entry is needed. The only subtlety is the construction-time vs write-time binding noted above.

Additional information

Suggested in the PR #327 review as "optional, not for this PR". Opened as its own issue so the follow-up actually exists instead of being politely mentioned by everyone and done by no one.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancement[issue/PR] requests / implements new or improved functionality.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions