feat: Add TUnitSettings static API for programmatic configuration#5522
feat: Add TUnitSettings static API for programmatic configuration#5522
Conversation
…Settings (#5521) Mark the Defaults class and all its fields as [Obsolete], pointing users to TUnitSettings.Timeouts.* equivalents. Migrate all internal references from Defaults.* to Settings.TUnitSettings.Timeouts.* to eliminate CS0618 warnings under TreatWarningsAsErrors. Also fix a namespace collision in NuGetDownloader.cs caused by the new TUnit.Core.Settings namespace.
There was a problem hiding this comment.
Code Review: TUnitSettings Static API for Programmatic Configuration
This is a well-structured PR that addresses a real usability gap — giving users a code-level alternative to CLI flags and environment variables. The docs are thorough, the [Obsolete] migration path from Defaults is clean, and the public API snapshots are correctly updated. Good work overall.
That said, I have a few architectural concerns worth discussing before merging.
1. Lifecycle Timing: Some Settings Will Be Silently Ignored
TUnitServiceProvider.cs — FailFast is captured once at construction:
var isFailFastEnabled = CommandLineOptions.TryGetOptionArgumentList(FailFastCommandProvider.FailFast, out _)
|| TUnitSettings.Execution.FailFast;This is evaluated once at TUnitServiceProvider construction. If TUnitServiceProvider is constructed before the user's [Before(HookType.TestDiscovery)] hook runs, setting TUnitSettings.Execution.FailFast = true in a discovery hook will have no effect — silently.
The same concern applies to BannerCapability and DisableLogo. The banner fires early in the test session (before discovery begins), so a discovery hook would be too late for that setting.
Recommendation: Either document that specific settings (Display.DisableLogo, Execution.FailFast) must be set before the test host starts — which isn't really possible via the hook API — or redesign those to read lazily. For isFailFastEnabled, passing it through the FailFastCancellationSource lazily (checking the setting at the point of failure, not at construction) would be more robust.
2. Eager Value Capture in HookMethod.Timeout
// TUnit.Core/Hooks/HookMethod.cs
public TimeSpan? Timeout { get; internal set; } = Settings.TUnitSettings.Timeouts.DefaultHookTimeout;This captures the value of DefaultHookTimeout at HookMethod construction time, not at execution time. If TUnit constructs HookMethod instances during discovery before the user's BeforeTestDiscovery hook runs, the custom hook timeout will never be applied — same silent-failure mode as above.
The same issue exists in TestBuilder.cs and TestBuilderPipeline.cs where TUnitSettings.Timeouts.DefaultTestTimeout is assigned to Timeout at construction. If test objects are built before the discovery hook fires, the customized timeout is invisible.
Recommendation: Consider a lazy resolution — e.g. Timeout ?? TUnitSettings.Timeouts.DefaultHookTimeout at the point of use rather than baking in the value at construction.
3. Negative MaximumParallelTests Falls Through Silently
// TUnit.Engine/Scheduling/TestScheduler.cs
if (codeLimit == 0)
{
return int.MaxValue; // unlimited
}
if (codeLimit > 0)
{
return codeLimit;
}
// negative value: falls through to CPU-based defaultA user passing -1 thinking it means "unlimited" (like some APIs do) gets the CPU-based default silently. This is confusing. Either treat negative values as unlimited, or throw an ArgumentOutOfRangeException in the setter of ParallelismSettings.MaximumParallelTests to fail fast with a clear message.
4. Test State Isolation Relies on Execution Order
// TUnit.UnitTests/TUnitSettingsTests.cs
[NotInParallel]
public class TUnitSettingsTests
{
[Test]
public async Task Defaults_Are_Correct() { ... } // no cleanup
[Test]
public async Task Settings_Can_Be_Modified()
{
var originalTimeout = TUnitSettings.Timeouts.DefaultTestTimeout;
try { ... }
finally { TUnitSettings.Timeouts.DefaultTestTimeout = originalTimeout; }
}
}Defaults_Are_Correct doesn't restore values after itself. If any external code (other tests in this run, or a global discovery hook) has mutated settings before this test runs, it will produce a false negative. [NotInParallel] prevents concurrent access but doesn't guarantee the settings are in their default state when the test starts.
Recommendation: Use a [Before] / [After] on the class to snapshot and restore all TUnitSettings values, not just the one being tested. Or, consider a TUnitSettings.ResetToDefaults() helper (internal is fine) specifically for testing infrastructure.
5. Public Mutability of Sub-Settings Objects
The sub-settings objects (DisplaySettings, ExecutionSettings, etc.) are publicly constructible sealed classes with public setters, but they're only ever accessed through the TUnitSettings static properties. This means:
- Users can store a reference to
TUnitSettings.Timeoutsand mutate it from anywhere, not just a discovery hook. - There's no enforcement of the "set in discovery hook only" contract.
This is a DX trade-off rather than a blocker, but worth flagging. A future consideration might be adding an IsLocked flag set after discovery that throws InvalidOperationException if settings are mutated mid-run.
What's Working Well
- The namespace collision fix in
NuGetDownloader.cs(NuGet.Configuration.Settings.LoadDefaultSettings) is a clean, necessary fix for the new namespace. - The
[Obsolete]deprecation onDefaultswith precise redirect messages is exactly right — users will get actionable compiler warnings. - The engine integration table in the PR description is genuinely helpful for reviewers.
- Documentation is high quality with the precedence rules clearly spelled out.
- All four TFM public API snapshots updated correctly.
Summary
| Severity | Nature | |
|---|---|---|
| FailFast / DisableLogo timing | Medium | Silently broken if set in discovery hook |
| HookMethod/TestBuilder eager capture | Medium | Custom timeout may not apply |
| Negative MaximumParallelTests | Low | Silent mismatch with user expectation |
| Test state isolation | Low | Fragile test, not a production bug |
| Public mutability enforcement | Low | DX / future concern |
The timing issue (point 1) is the most likely to produce a real bug that's hard to debug. I'd recommend verifying the construction order of TUnitServiceProvider relative to BeforeTestDiscovery hooks before shipping, and documenting any settings that cannot be configured via the hook API.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| UnusedCode | 1 medium |
| CodeStyle | 1 minor |
🟢 Metrics 12 complexity
Metric Results Complexity 12
TIP This summary will be updated as you push new changes. Give us feedback
…aths (#5521) - Defaults.cs fields now delegate to TUnitSettings.Timeouts (single source of truth) - Add `using TUnit.Core.Settings` to TUnit.Core files for consistent shorter access
There was a problem hiding this comment.
Code Review: feat: Add TUnitSettings static API for programmatic configuration
Overall this is a clean, well-structured addition that fills a real gap — version-controlled, IDE-discoverable settings are strictly better than invisible CLI flags. The grouped sub-objects (Timeouts, Parallelism, Display, Execution) are a solid design. A few issues worth addressing:
Bug: Defaults.* fields diverge from live TUnitSettings values
// TUnit.Core/Defaults.cs
public static readonly TimeSpan TestTimeout = TUnitSettings.Timeouts.DefaultTestTimeout;static readonly fields are evaluated once, at the time the Defaults type is first accessed. After that snapshot, any change to TUnitSettings.Timeouts.DefaultTestTimeout is invisible through Defaults.TestTimeout. The two values are now permanently decoupled.
This matters in a subtle way: if a user sets TUnitSettings.Timeouts.DefaultTestTimeout = TimeSpan.FromMinutes(10) in a [Before(HookType.TestDiscovery)] hook and then reads Defaults.TestTimeout (e.g., in custom code that hasn't migrated yet), they'll still see 30 minutes. The deprecated shim looks consistent but silently lies.
Recommended fix: use a static property instead of a readonly field to delegate live reads:
[Obsolete("...")]
public static TimeSpan TestTimeout => TUnitSettings.Timeouts.DefaultTestTimeout;This keeps backward-compat callers and live state in sync during the deprecation window.
Unintuitive 0 = unlimited semantic for MaximumParallelTests
In TestScheduler.GetMaxParallelism:
if (codeLimit == 0)
{
logger.LogDebug("Maximum parallel tests: unlimited (from TUnitSettings)");
return int.MaxValue;
}Setting a property to 0 meaning "unlimited" is non-obvious. The documentation correctly notes null means "use the default heuristic" but doesn't document the 0 special case at all. Negative values also silently fall through to the CPU default with no warning or error.
The CLI/env-var path likely has the same convention (worth verifying), but for a code API the intent is much less clear. Consider:
- Using
null= default, any positive value = explicit limit, and removing the0special case (or documenting it prominently) - Or validating and throwing
ArgumentOutOfRangeExceptionfor negative values instead of silently ignoring them
HookMethod.Timeout captures a snapshot, not a live value
// TUnit.Core/Hooks/HookMethod.cs
public TimeSpan? Timeout { get; internal set; } = TUnitSettings.Timeouts.DefaultHookTimeout;This reads DefaultHookTimeout once when the record is instantiated. If hooks are constructed before the [Before(HookType.TestDiscovery)] hook fires and sets a custom timeout, the default won't apply. This is likely fine in practice (hooks are typically registered after discovery configuration), but it creates a subtle ordering dependency that isn't documented.
Test isolation: [NotInParallel] is necessary but the underlying concern is real
[NotInParallel]
public class TUnitSettingsTests
{
[Test]
public async Task Settings_Can_Be_Modified()
{
var originalTimeout = TUnitSettings.Timeouts.DefaultTestTimeout;
try
{
TUnitSettings.Timeouts.DefaultTestTimeout = TimeSpan.FromMinutes(10);
...
}
finally
{
TUnitSettings.Timeouts.DefaultTestTimeout = originalTimeout;
}
}
}[NotInParallel] prevents concurrency within the test assembly, but TUnitSettings is a process-global singleton. If other test assemblies run in the same process and have their own [Before(HookType.TestDiscovery)] hooks that set values, those values will leak into this test's assertions. The Defaults_Are_Correct test in particular will be fragile in a multi-assembly test run.
This is more of a documentation/design note than a bug — the right answer is probably to note in the docs that TUnitSettings is process-wide global state.
Architectural observation: global mutable static vs DI
The global static pattern is pragmatic given the [Before(HookType.TestDiscovery)] hook constraint — there's nowhere to inject a settings object that early. That said, the existing TUnitServiceProvider DI tree already passes CommandLineOptions and EnvironmentConstants through constructor injection. If a TUnitConfiguration instance (holding the merged/resolved values) were passed through DI, it would be testable in isolation and avoid the global-state coupling.
This is a larger architectural change and may not be worth the churn now, but worth considering as the settings surface grows.
Minor: NuGetDownloader.cs change is unrelated but correct
Qualifying NuGet.Configuration.Settings fully to resolve the name collision is the right fix. No concerns.
Summary
| Issue | Severity |
|---|---|
Defaults.* readonly fields don't track live TUnitSettings changes |
Bug |
MaximumParallelTests = 0 → unlimited is undocumented; negatives silently ignored |
Significant |
HookMethod.Timeout snapshot ordering dependency |
Minor / needs documentation |
| Global mutable state leaks across test assemblies | Minor / needs documentation |
The overall structure and documentation quality are good. Addressing the readonly → property change and the MaximumParallelTests validation/documentation would make this solid.
…idate parallelism (#5521) - Remove DisplaySettings.DisableLogo (banner fires before discovery hooks, making it useless as a code setting) - Make FailFast check lazy in TestRunner (reads TUnitSettings.Execution.FailFast at failure time, not at construction) - Add setter validation on MaximumParallelTests (reject negative values) - Document that MaximumParallelTests is read before discovery hooks - Update public API snapshots and docs
There was a problem hiding this comment.
Code Review: feat: Add TUnitSettings static API for programmatic configuration
Overall, this is a well-structured PR that adds a genuinely useful API surface. The grouped sub-objects (Timeouts, Parallelism, Display, Execution) are discoverable and make intent clear. Snapshot tests are updated and the deprecation of Defaults is handled correctly. A few issues worth addressing before merge:
Issue 1 (Bug): Display.DisableLogo is mentioned in the PR description but never implemented
The PR body's table lists:
Display.DisableLogo→BannerCapability
But DisplaySettings.cs only has DetailedStackTrace — there is no DisableLogo property, and BannerCapability is not modified in the diff. Either the table is aspirational (in which case the PR body should call it out as a follow-up), or the feature was accidentally omitted.
Issue 2 (Bug): FailFast is checked redundantly and inconsistently
In TUnitServiceProvider.cs:
var isFailFastEnabled = CommandLineOptions.TryGetOptionArgumentList(FailFastCommandProvider.FailFast, out _)
|| TUnitSettings.Execution.FailFast; // ← captured at construction timeThen in TestRunner.cs, the same setting is re-evaluated at execution time:
if ((_isFailFastEnabled || TUnitSettings.Execution.FailFast) && test.Result?.State == TestState.Failed)Since _isFailFastEnabled already incorporates TUnitSettings.Execution.FailFast at construction, the || TUnitSettings.Execution.FailFast in TestRunner is a no-op (assuming the setting isn't mutated mid-run — if it is supposed to be dynamic, the construction-time capture in TUnitServiceProvider is wrong instead).
Recommendation: Pick one approach. If the setting is read once at startup (the simpler and safer model), remove the dynamic re-check in TestRunner. If it needs to be dynamic, don't OR it into _isFailFastEnabled at all — just always read it from TUnitSettings.Execution.FailFast at the point of use.
Issue 3 (Subtle Bug): Defaults backward-compat is broken for mutation
// Defaults.cs
public static readonly TimeSpan TestTimeout = TUnitSettings.Timeouts.DefaultTestTimeout;This captures the value at class initialization time. If a user (correctly) configures:
TUnitSettings.Timeouts.DefaultTestTimeout = TimeSpan.FromMinutes(5);...then reads Defaults.TestTimeout, they will still see 30 minutes. The field is frozen at startup. This could silently mislead anyone who still reads from Defaults in their own code.
Since Defaults is now [Obsolete], this is a minor concern — but the field now carries the false promise of mirroring TUnitSettings. It would be clearer to leave the old hard-coded values in place (the obsolete attribute already points users to the new API) rather than creating a live-but-stale alias.
Issue 4 (Usability Trap): MaximumParallelTests is documented as incompatible with the recommended hook pattern
The ParallelismSettings XML doc correctly warns that MaximumParallelTests is read before [Before(HookType.TestDiscovery)] hooks run, but the PR description's example shows all settings being set inside such a hook:
[Before(HookType.TestDiscovery)]
public static Task Configure(BeforeTestDiscoveryContext context)
{
TUnitSettings.Parallelism.MaximumParallelTests = 4; // ← silently ignored!This will silently do nothing for parallelism. The usability concern here is real — users who follow the PR's own example will be confused when parallelism doesn't change. Consider either:
- Raising an
InvalidOperationException(or logging a warning) whenMaximumParallelTestsis set after the scheduler has already initialized, or - Deferring scheduler initialization so it reads the setting after discovery hooks run
Issue 5 (Design): No reset/isolation mechanism for the static singleton
TUnitSettings is a static class holding mutable singletons. There is no Reset() or internal seam to restore defaults. This matters for:
- Unit tests of TUnit itself —
TUnitSettingsTestsmutates the global singleton; if tests run in parallel they can interfere with each other. - Multi-session scenarios (e.g., process reuse in IDE test runners) — settings from one run bleed into the next.
Consider adding an internal static void Reset() (or use a [ModuleInitializer] guard) so tests and the engine can restore known-good state.
Minor Observations
NuGetDownloader.cschange (NuGet.Configuration.Settings.LoadDefaultSettings(null)): Unrelated to the feature but a legitimate disambiguation fix — fine to include.ParallelismSettingssetter validation: The guardif (value < 0)is correct and matches the doc. Theif (codeLimit > 0)branch inTestScheduleris always true once the null/zero cases are handled — consider removing the dead branch to make the intent clearer.- Public API snapshots: All 4 TFMs updated — good.
- Docs: New
programmatic-configuration.mdis a welcome addition.
Summary: The architecture is sound and the API design is clean. The main things to fix before merge are the missing DisableLogo feature (or clarifying it's out of scope), the redundant FailFast check in TestRunner, and adding a warning/guard for the MaximumParallelTests-in-discovery-hook footgun.
…aults alias, fix docs (#5521) - Remove TUnitSettings.Execution.FailFast from TUnitServiceProvider (eager capture). TestRunner already checks it lazily at failure time — no double-check needed. - Revert Defaults.cs to hardcoded values (static readonly fields can't track mutable TUnitSettings; stale alias is worse than honest deprecation). - Remove MaximumParallelTests from discovery hook example in docs (silently ignored due to scheduler timing) and add a note about the limitation.
There was a problem hiding this comment.
Follow-Up Review: feat: Add TUnitSettings static API for programmatic configuration
Good progress since the prior reviews. Most of the significant issues raised have been addressed.
Previous Issues: Status Check
| Issue | Status |
|---|---|
| FailFast redundancy — eager capture in TUnitServiceProvider | ✅ Fixed: TUnitServiceProvider now only captures CLI flag; TestRunner checks TUnitSettings.Execution.FailFast lazily |
| Negative MaximumParallelTests silently falls through | ✅ Fixed: setter throws ArgumentOutOfRangeException |
| Defaults.* stale alias | ✅ Fixed: reverted to hardcoded values + [Obsolete]; honest deprecation |
| Display.DisableLogo in API | ✅ Correctly removed — banner fires before any user code |
| programmatic-configuration.md missing | ✅ Added |
Remaining Issues
1. parallelism.md example contradicts scheduler timing (misleading docs)
The commit message for the latest commit says "Remove MaximumParallelTests from discovery hook example in docs", but the example is still present in the file at the PR head:
// docs/docs/execution/parallelism.md
[Before(HookType.TestDiscovery)]
public static Task Configure(BeforeTestDiscoveryContext context)
{
TUnitSettings.Parallelism.MaximumParallelTests = 4; // ← silently ignored
return Task.CompletedTask;
}programmatic-configuration.md correctly warns that MaximumParallelTests is read before discovery hooks run. But parallelism.md presents this exact pattern as the canonical usage. A user who lands on parallelism.md (the more likely discovery path) will follow the example, see no effect, and have no idea why.
Recommendation: Remove or replace this example with the CLI/env-var approach, or at minimum add the same warning note inline: MaximumParallelTests must be set via a module initializer or static constructor, not a discovery hook.
2. programmatic-configuration.md "When to Set" contradicts MaximumParallelTests
## When to Set
Always set TUnitSettings values inside a [Before(HookType.TestDiscovery)] hook.This appears six lines after a table entry that explicitly says MaximumParallelTests cannot be set in a discovery hook. The blanket "always" is wrong for that one setting and will confuse users who read both sections.
Recommendation: Qualify the advice: "Set most TUnitSettings values inside a [Before(HookType.TestDiscovery)] hook. The exception is Parallelism.MaximumParallelTests — see the note above."
3. HookMethod.Timeout still eagerly captures at construction
// TUnit.Core/Hooks/HookMethod.cs
public TimeSpan? Timeout { get; internal set; } = TUnitSettings.Timeouts.DefaultHookTimeout;This was flagged in prior reviews and is unchanged. The value is baked in when the HookMethod record is instantiated. If hook registration happens before the user's [Before(HookType.TestDiscovery)] hook fires, any custom DefaultHookTimeout is silently ignored — same ordering problem as MaximumParallelTests.
Recommendation: Either resolve lazily (Timeout ?? TUnitSettings.Timeouts.DefaultHookTimeout at execution time), or document the ordering constraint in programmatic-configuration.md so users know this setting may not be configurable via the hook API.
4. TUnitSettingsTests.Defaults_Are_Correct has no state guard
[Test]
public async Task Defaults_Are_Correct()
{
await Assert.That(TUnitSettings.Timeouts.DefaultTestTimeout).IsEqualTo(TimeSpan.FromMinutes(30));
// no setup, no restore
}[NotInParallel] prevents concurrent writes but doesn't guarantee pristine state at test entry. If any code in the process run has mutated TUnitSettings before this test executes, it will produce a false negative. This was raised in two prior reviews and remains.
Recommendation: Add a class-level [Before] / [After] pair that snapshots and restores all TUnitSettings values, or an internal TUnitSettings.ResetToDefaults() helper.
What's Working Well
The overall structure has improved significantly through revision. The lazy FailFast check in TestRunner is clean. Reverting Defaults.cs to hardcoded values with [Obsolete] is the right call — it's honest and avoids a stale alias. The new programmatic-configuration.md is thorough. Public API snapshots are correct across all four TFMs.
Summary
| Issue | Severity |
|---|---|
parallelism.md shows MaximumParallelTests in discovery hook (should have been removed per commit message) |
Medium — actively misleads users |
programmatic-configuration.md "When to Set" contradicts MaximumParallelTests limitation |
Low — doc inconsistency |
HookMethod.Timeout eager capture undocumented ordering constraint |
Low — silent failure potential |
TUnitSettingsTests.Defaults_Are_Correct has no state guard |
Low — fragile test |
The parallelism.md issue is the one most worth fixing before merge — it contradicts correct behavior and the commit that was meant to address it appears not to have landed.
…dering (#5521) - Replace discovery-hook example in parallelism.md with CLI/env-var approach (MaximumParallelTests is read before discovery hooks run) - Qualify "When to Set" section to note MaximumParallelTests exception - Document DefaultHookTimeout ordering constraint in XML doc
Summary
Closes #5521
Closes #5523
TUnitSettingsclass exposed viaBeforeTestDiscoveryContext.Settings, giving users a discoverable, code-level configuration API that naturally enforces the correct lifecycle timingTimeouts,Parallelism,Execution,Display)context.Settings> built-in defaultDefaultsclass with[Obsolete]pointing to the new APIMaximumParallelTests(no negatives)MaximumParallelTestsread viaLazy<T>so it works from discovery hooksNew API
TUnitSettingsis a sealed class with aninternalconstructor andinternal static Defaultsingleton. Users access settings exclusively throughcontext.Settingsin a[Before(HookType.TestDiscovery)]hook. Engine code usesTUnitSettings.Default.*viaInternalsVisibleTo.Engine integration points
Timeouts.DefaultTestTimeoutTestBuilderPipeline,TestBuilder,DedicatedThreadExecutorTimeouts.DefaultHookTimeoutHookMethodTimeouts.ForcefulExitTimeoutEngineCancellationTokenTimeouts.ProcessExitHookDelayEngineCancellationTokenParallelism.MaximumParallelTestsTestScheduler.GetMaxParallelism(lazy)Execution.FailFastTestRunner(lazy, per-failure)Display.DetailedStackTraceTUnitMessageBus(lazy, per-failure)What's intentionally excluded
Display.DisableLogo— The banner fires during test host startup, before any user code runs. Cannot be configured via code; use--disable-logoorTUNIT_DISABLE_LOGO.Test plan
TUnitSettingsTestspass (defaults correct, properties mutable, state isolation via Before/After hooks)Defaultsclass delegates toTUnitSettings.Default(no stale divergence)FailFastreads lazily inTestRunner(works from discovery hooks)MaximumParallelTestsreads lazily viaLazy<T>inTestScheduler(works from discovery hooks)MaximumParallelTestsvalidates against negative valuesTimeoutSettingsvalidates against zero/negative durations (ProcessExitHookDelayallows zero)NuGet.Configuration.Settingsnamespace collision fixed inNuGetDownloader.cs