feat(playwright): add TUnitPlaywrightSettings defaults

[thomhurst]

Summary

• Add TUnitPlaywrightSettings with DefaultHeadless and DefaultIgnoreHttpsErrors knobs, exposed via a TUnitSettings.PlaywrightSettings extension property
• Wire defaults into BrowserFixture.GetLaunchOptions, BrowserTest ctor, ContextFixture.GetContextOptions, and ContextTest.ContextOptions

Test plan

• dotnet build TUnit.Playwright
• Verify Playwright sample tests still launch with expected headless/HTTPS behavior

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review

The feature intent is clear and useful, but there's one breaking change and an architectural inconsistency worth addressing.

---

🔴 Breaking behavior change: DefaultHeadless defaults to false

File: TUnit.Playwright/PlaywrightSettings.cs

public bool DefaultHeadless { get; set; }  // bool default = false

Playwright's runtime default for Headless is true. Previously, GetLaunchOptions() => new() left Headless unset (null), so Playwright launched headlessly by default. After this PR, every BrowserFixture and BrowserTest will inject Headless = false (headed mode) unless users explicitly opt out.

This will break CI pipelines where tests ran headlessly without explicit configuration — a significant silent regression.

Fix: Either default to true to match Playwright's own default, or use bool? so null means "let Playwright decide":

public class TUnitPlaywrightSettings
{
    public bool? Headless { get; set; }       // null = use Playwright's default (true)
    public bool? IgnoreHttpsErrors { get; set; }
}

Then in GetLaunchOptions():

protected virtual BrowserTypeLaunchOptions GetLaunchOptions() => new()
{
    Headless = TUnitSettings.Default.PlaywrightSettings.Headless,  // null is valid; Playwright treats null as true
};

This preserves existing behaviour for users who haven't configured anything, and is explicit about intent.

---

🟡 Architectural mismatch with TUnitSettings pattern

All other settings sections (Timeouts, Parallelism, Display, Execution) are instance-owned properties of TUnitSettings. PlaywrightSettings is instead backed by a global static readonly singleton in PlaywrightSettingsExtensions, which isn't per-instance and bypasses the settings hierarchy.

Current (inconsistent):

// PlaywrightSettings.cs
public static class PlaywrightSettingsExtensions
{
    internal static readonly TUnitPlaywrightSettings Default = new();  // global singleton

    extension(TUnitSettings settings) {
        public TUnitPlaywrightSettings PlaywrightSettings => Default;  // always same object regardless of 'settings'
    }
}

// BrowserFixture.cs
Headless = PlaywrightSettingsExtensions.Default.DefaultHeadless,  // bypasses TUnitSettings entirely

Better (consistent with existing pattern):

// In TUnitSettings.cs — add PlaywrightSettings as a direct property like Timeouts/Parallelism
public TUnitPlaywrightSettings PlaywrightSettings { get; } = new();

// TUnitPlaywrightSettings.cs — give it an internal ctor like the other settings classes
public sealed class TUnitPlaywrightSettings
{
    internal TUnitPlaywrightSettings() { }
    public bool? Headless { get; set; }
    public bool? IgnoreHttpsErrors { get; set; }
}

// BrowserFixture.cs — use the canonical path through TUnitSettings
Headless = TUnitSettings.Default.PlaywrightSettings.Headless,

This would mean TUnitSettings needs a dependency on TUnit.Playwright, which isn't ideal since TUnit.Core shouldn't know about Playwright. The extension approach does solve that, but the static-singleton backing is what creates the inconsistency.

An alternative that keeps the decoupling: keep the extension property, but have it return an instance stored on the TUnitSettings instance using ConditionalWeakTable<TUnitSettings, TUnitPlaywrightSettings> or similar, so the settings are properly scoped. That said, given there is only ever one TUnitSettings.Default in practice, the current approach does function correctly — it's more a matter of conceptual cleanliness.

---

🟡 Naming: Default prefix is redundant

DefaultHeadless and DefaultIgnoreHttpsErrors inside a class called TUnitPlaywrightSettings (which already represents defaults) could just be Headless and IgnoreHttpsErrors. Compare: settings.Timeouts.DefaultTestTimeout — that one says Default because there are non-default timeouts on the same object. Here everything is a default.

---

✅ What's good

• Wiring the settings into both BrowserFixture and BrowserTest code paths is correct — they both need consistent options.
• ContextFixture and ContextTest are both updated, keeping the two paths in sync.
• Public API snapshot files are updated, which is exactly right.
• The C# 14 extension block for cross-assembly settings extension is a smart way to avoid a circular dependency between TUnit.Core and TUnit.Playwright.

The main ask is to fix the DefaultHeadless = false regression before merging.

[claude]

Code Review — Follow-up on Fix commit

The previous concerns have been partially addressed. Here's where things stand.

---

✅ Breaking change fixed

DefaultHeadless is now bool? — passing null to BrowserTypeLaunchOptions.Headless lets Playwright use its own default (true). That regression is resolved.

---

🔴 Snapshots are stale after the Fix commit

Commit e97914f1e8 changed the public API surface of TUnitPlaywrightSettings, but the .verified.txt snapshot files were not regenerated. The current snapshots still show the pre-fix API:

Snapshot says:

public class TUnitPlaywrightSettings
{
    public TUnitPlaywrightSettings() { }    // ctor was public before Fix
    public bool DefaultHeadless { get; set; }  // bool, not bool?
    ...
}

Actual code after Fix:

public class TUnitPlaywrightSettings
{
    internal TUnitPlaywrightSettings() { }   // now internal
    public bool? DefaultHeadless { get; set; } = null;  // now bool?
    ...
}

The snapshot tests will fail on CI. Regenerate them from the TUnit.PublicAPI test project:

cd TUnit.PublicAPI
dotnet test
# accept received files:
for f in *.received.txt; do mv "$f" "${f%.received.txt}.verified.txt"; done
git add *.verified.txt

---

🟡 DefaultIgnoreHttpsErrors inconsistency

DefaultHeadless was correctly made bool? so unset means "use Playwright's default." DefaultIgnoreHttpsErrors is still bool (defaults to false). While false happens to match the Playwright default, the asymmetry is confusing — one says "I don't care, use your default" and the other says "I explicitly want false." Consider making it bool? too for consistency:

public bool? DefaultIgnoreHttpsErrors { get; set; }

Both ContextFixture and ContextTest would then pass it as-is, since BrowserNewContextOptions.IgnoreHTTPSErrors is also bool?.

---

🟡 Naming: Default prefix is still redundant

DefaultHeadless / DefaultIgnoreHttpsErrors inside a class named TUnitPlaywrightSettings is repetitive — everything on the class is already a default. The equivalent in TUnitTimeoutSettings is DefaultTestTimeout, but that object also has non-default counterparts. Here, all properties represent defaults.

Consider renaming to Headless and IgnoreHttpsErrors before this ships, since renaming after a public release is a breaking change.

---

✅ Architectural improvement noted

Moving Default from PlaywrightSettingsExtensions into TUnitPlaywrightSettings itself is cleaner. The extension now has a single responsibility (providing the cross-assembly extension property), and the singleton lives with the type it belongs to.

---

Summary: The breaking-change regression is fixed — good. The remaining blocker is the stale snapshot files. The bool? consistency for DefaultIgnoreHttpsErrors and the naming question are worth resolving before the first public release of this API, since both would be breaking changes to fix later.

[claude]

Code Review — Round 3

All three issues from the previous reviews have been addressed. Here's the current status:

✅ All prior blockers resolved

Issue	Status
DefaultHeadless was non-nullable bool (breaking CI)	Fixed — now bool?
DefaultIgnoreHttpsErrors was non-nullable bool	Fixed — now bool?
Stale snapshot files after the fix commit	Fixed — snapshots now show bool? and no public constructor

The snapshots correctly omit the constructor (because it's internal) and show both properties as bool? — exactly right.

---

🟡 Remaining: Default prefix in property names (pre-existing from both prior reviews)

DefaultHeadless and DefaultIgnoreHttpsErrors still carry the Default prefix, which is redundant given the class name is TUnitPlaywrightSettings (every property here is already a default). This was flagged in both round 1 and round 2 without being acted on.

This matters because it's a public API — renaming after release is a breaking change. Compare how TimeoutSettings handles it: it uses DefaultTestTimeout etc. because the same object also holds non-default counterparts. TUnitPlaywrightSettings has no such need, so the prefix adds noise without adding signal.

The cleanest fix before this ships:

public bool? Headless { get; set; }
public bool? IgnoreHttpsErrors { get; set; }

If you prefer to keep DefaultHeadless to signal "this is a run-wide default, not a per-test override," that's a valid call — but it should be a conscious decision rather than carried over by accident. Either way, the implementation is solid and the regression is fixed.