Skip to content

feat(playwright): expose default Context/Launch options on settings#5861

Merged
thomhurst merged 1 commit intomainfrom
feat/playwright-default-options
May 8, 2026
Merged

feat(playwright): expose default Context/Launch options on settings#5861
thomhurst merged 1 commit intomainfrom
feat/playwright-default-options

Conversation

@thomhurst
Copy link
Copy Markdown
Owner

Summary

  • Replace granular DefaultHeadless / DefaultIgnoreHttpsErrors on TUnitPlaywrightSettings with full options objects: DefaultBrowserNewContextOptions and DefaultBrowserTypeLaunchOptions.
  • One extension point now covers every Playwright option without adding a new property each time.
  • When a defaults object is set, it fully replaces the hardcoded base defaults (Locale = "en-US", ColorScheme = Light); when null, base defaults still apply.

Breaking changes

Test plan

  • Public API snapshots regenerated and accepted across net472 / net8.0 / net9.0 / net10.0.
  • CI green.

Replaces the granular DefaultHeadless/DefaultIgnoreHttpsErrors props
on TUnitPlaywrightSettings with full options objects:
DefaultBrowserNewContextOptions and DefaultBrowserTypeLaunchOptions.
A single extension point now covers every Playwright option without
adding a new property each time. When set, the option object fully
replaces the hardcoded base defaults (Locale=en-US, ColorScheme=Light).

BREAKING: removes DefaultHeadless and DefaultIgnoreHttpsErrors.
Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Good direction overall — full options objects scale far better than adding a property per Playwright flag. The API snapshot updates are clean and consistent across all targets. A few concerns worth addressing:


1. All-or-nothing replacement is a usability regression

The problem: Users who only want to set IgnoreHTTPSErrors = true must now reconstruct the entire BrowserNewContextOptions, including remembering to re-apply Locale = "en-US" and ColorScheme = Light themselves. If they forget, the locale/color-scheme defaults silently vanish.

The old API (DefaultIgnoreHttpsErrors = true) was more ergonomic for single-option overrides. The new one is more powerful but less safe.

Better approach — a configurer callback:

// In TUnitPlaywrightSettings:
public Action<BrowserNewContextOptions>? ConfigureContextOptions { get; set; }

// Applied at use site:
protected virtual BrowserNewContextOptions GetContextOptions()
{
    var opts = new BrowserNewContextOptions { Locale = "en-US", ColorScheme = ColorScheme.Light };
    TUnitPlaywrightSettings.Default.ConfigureContextOptions?.Invoke(opts);
    return opts;
}

This preserves the hardcoded sensible defaults and lets users layer on top without having to know what the base defaults are. The same pattern applies to ConfigureLaunchOptions. It also eliminates the null vs "user explicitly wants empty options" ambiguity.


2. Shared mutable state risk

TUnitPlaywrightSettings.Default.DefaultBrowserTypeLaunchOptions returns the same object instance to every caller. A subclass or test that mutates it (e.g. options.Headless = true after retrieving it) would affect all concurrent or subsequent tests.

In BrowserTest.cs:

public BrowserTest() : this(TUnitPlaywrightSettings.Default.DefaultBrowserTypeLaunchOptions ?? new BrowserTypeLaunchOptions())

The reference is passed directly into the base constructor. If Playwright or user code later mutates that object, you have a shared-state bug.

Fix: Clone on read, not on write. Either clone the options object at the call site (Playwright's options classes are not ICloneable, so a copy constructor or shallow-clone helper is needed), or use the callback pattern from point 1 (which constructs a fresh object each time).


3. Duplicated fallback defaults between ContextFixture and ContextTest

Both ContextFixture.GetContextOptions() and ContextTest.ContextOptions() have identical fallback logic:

new BrowserNewContextOptions { Locale = "en-US", ColorScheme = ColorScheme.Light }

This duplication pre-existed but is now load-bearing: if someone adds another default in one place and forgets the other, behavior diverges silently. A shared static BrowserNewContextOptions CreateDefaultContextOptions() helper would make this a single source of truth.


4. Redundant nullable initializers (minor)

public BrowserTypeLaunchOptions? DefaultBrowserTypeLaunchOptions { get; set; } = null;
public BrowserNewContextOptions? DefaultBrowserNewContextOptions { get; set; } = null;

= null is the default for nullable reference types and can be removed.


Summary: The concept is right and worth shipping over the granular-property approach. The main ask before merging is either (a) adopt the callback pattern to avoid the silent-defaults-loss problem, or (b) at minimum document clearly in the XML docs that setting these replaces all defaults, so users know to re-specify Locale/ColorScheme themselves. The shared-mutable-state issue is a correctness concern worth addressing alongside.

@codacy-production
Copy link
Copy Markdown

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant