feat(playwright): add composition-based fixtures#5840
feat(playwright): add composition-based fixtures#5840
Conversation
Adds PlaywrightFixture, BrowserFixture, ContextFixture, and PageFixture as an additive alternative to the inheritance-based PageTest hierarchy. Each implements IAsyncInitializer/IAsyncDisposable and composes via [ClassDataSource<T>] property injection, letting users mix shared scopes (e.g. PerTestSession browser, per-test context) without inheriting from TUnit base classes and without conflicting with their own base classes. Multi-account scenarios work naturally: two [ClassDataSource<PageFixture>] properties yield two distinct IBrowserContext while sharing one IBrowser via the PerTestSession scope on BrowserFixture. Extracts Playwright Service launch logic into PlaywrightServiceConnector and W3C trace-header merge into PlaywrightTelemetryHeaders so both the inheritance hierarchy (BrowserService, BrowserTest) and the new fixtures share the same implementation. Smoke test in the template project covers two-context storage isolation and verifies the underlying IBrowser is shared.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 14 |
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[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a well-designed addition. The composition-based fixture API is a natural complement to the inheritance hierarchy, and the shared-helper extraction (PlaywrightServiceConnector, PlaywrightTelemetryHeaders) is a clean, correct refactor.
Issue 1 — Misleading nullable annotation pattern (all four fixture files)
Every resource-owning fixture follows this pattern:
public IBrowser Browser { get; private set; } = null!;
public virtual async ValueTask DisposeAsync()
{
if (Browser is not null) // ← contradicts null!
await Browser.CloseAsync().ConfigureAwait(false);
}= null! tells the compiler "trust me, this will never be null," but the if (X is not null) guard immediately contradicts that promise. A reader has to hold both ideas at once: "the field is non-nullable… except it might be null at dispose time." The same tension appears in PlaywrightFixture where Playwright?.Dispose() is used.
Why the suggested fix is better: Declaring the backing field as truly nullable (T?) makes the intent explicit: "this starts unset and is assigned during InitializeAsync." The guard in DisposeAsync then reads naturally as a defensive check for the case where init never ran (e.g., an exception before InitializeAsync completed).
// Preferred in all four fixtures:
public IBrowser? Browser { get; private set; }
public virtual async ValueTask DisposeAsync()
{
if (Browser is not null)
await Browser.CloseAsync().ConfigureAwait(false);
}The null! suppressor is fine when used to satisfy required init-only properties that are guaranteed set by the DI container — but for lazily-initialised resource fields it's the wrong tool.
Issue 2 — Hardcoded PerTestSession scope blocks customisation without subclassing workarounds
BrowserFixture and ContextFixture declare their dependencies at PerTestSession scope directly on their required properties:
// BrowserFixture.cs
[ClassDataSource<PlaywrightFixture>(Shared = SharedType.PerTestSession)]
public required PlaywrightFixture PlaywrightFixture { get; init; }
// ContextFixture.cs
[ClassDataSource<BrowserFixture>(Shared = SharedType.PerTestSession)]
public required BrowserFixture BrowserFixture { get; init; }Because these are init-only and the attribute is sealed at the property level, a user who wants PerClass or PerAssembly browser sharing cannot override the scope — they'd need to author an entirely new fixture class. Subclassing won't help because you can't replace an attribute on an inherited property.
Why this matters: Power users writing suites that need e.g. one browser per test class (expensive launch amortised across a class, isolated across classes) have no lever to pull without forking the fixtures.
Architectural suggestion: Consider whether the Shared value belongs as a default that the user overrides at the test-class injection site, or whether the scope should be exposed as a way to configure the fixture itself. One option is to document this limitation explicitly in XML doc comments so users know they need their own fixture class to change scope. Another option (more complex) is a marker interface or static factory that lets the user supply the scope when injecting BrowserFixture.
Even if you decide the current defaults are the right trade-off for v1, a doc comment stating the intended defaults and the workaround would prevent confusion.
Minor observation — PageFixture silently defaults to a new ContextFixture per test
PageFixture omits Shared = on its ContextFixture dependency:
[ClassDataSource<ContextFixture>] // no Shared = ...
public required ContextFixture ContextFixture { get; init; }This is intentional (it's what makes the two-Alice-and-Bob scenario work), and the TwoContextFixtureTests smoke test demonstrates it well. But it's asymmetric with the PerTestSession declarations further up the chain, and a user who injects two [ClassDataSource<PageFixture>] properties without reading the docs may be surprised to get two browser contexts.
A brief XML doc summary on PageFixture calling out this default would make the intent self-documenting without adding any runtime cost.
Strengths worth keeping
DateTime.UtcNowoverDateTime.Now.ToUniversalTime()inPlaywrightServiceConnector— correct and cheaper.- All public-facing lifecycle methods are
virtual, giving subclasses full control. PlaywrightFixture.Expect(…)overloads are a nice ergonomic touch.- The
TwoContextFixtureTestssmoke test is an excellent, runnable specification of the scoping contract. - Keeping the existing
PlaywrightTesthierarchy untouched avoids breaking changes.
…tRef + scope docs Template smoke test references new PageFixture/ContextFixture/BrowserFixture types added in this PR; CI's `dotnet restore` pulls TUnit.Playwright 1.43.x from nuget.org, which doesn't have them yet. Adds the same Directory.Build.props swap pattern already used for TUnit.Aspire so solution builds resolve to the local TUnit.Playwright project, while end-user template instantiations keep the floating PackageReference. Also documents the hardcoded SharedType on BrowserFixture/ContextFixture property attributes (cannot be overridden via subclassing — author a new fixture class to change scope) and the SharedType.None default on PageFixture that enables two-context-per-test scenarios.
|
Thanks for the review. Issue 1 ( Issue 2 (hardcoded scope on inherited init properties) — fair call, added doc comments to Minor (PageFixture default scope) — added the doc note explaining Also fixed the unrelated CI failure in the same commit: the template smoke test references new types added in this PR, but CI's restore pulls |
There was a problem hiding this comment.
Follow-up Review — Round 2
Previous findings: status check
Issue 1 (null! contradiction) — Author's pushback is reasonable: diverging from the established PlaywrightTest/BrowserTest/AspireFixture pattern inside a single PR would create two conventions. Accepting the deferral to a separate alignment PR. No further action needed here.
Issue 2 (hardcoded scope docs) — XML doc comments added to BrowserFixture and ContextFixture. ✓ Confirmed.
Minor (PageFixture default scope) — XML doc explaining SharedType.None is intentional. ✓ Confirmed. Clear and accurate.
New finding: ContextFixture.GetContextOptions applies opinionated defaults absent from the inheritance hierarchy
// ContextFixture.cs — new composition path
protected virtual BrowserNewContextOptions GetContextOptions() =>
new() { Locale = "en-US", ColorScheme = ColorScheme.Light };// BrowserTest.cs — existing inheritance path
public async Task<IBrowserContext> NewContext(BrowserNewContextOptions options)
{
options = PlaywrightTelemetryHeaders.Merge(options, PropagateTraceContext);
// no locale / colour-scheme defaults appliedBrowserTest leaves Locale and ColorScheme at the browser default. ContextFixture silently pins them to en-US / Light. A user migrating a test suite from ContextTest : BrowserTest to [ClassDataSource<ContextFixture>] will get different context behaviour without any indication — locale-sensitive assertions or media-query tests could start failing unexpectedly.
Suggested fix (pick one):
- Change the default to
new()(no pinned locale/colour) and note the override point in a doc comment — keeps parity with the inheritance path and makes the intent explicit. - Or keep the opinionated defaults but add an XML doc note on
GetContextOptionsdocumenting that these diverge from theBrowserTestbaseline and why they were chosen.
Either is fine; the second option is defensible if the defaults were deliberate UX hardening, but users need to know about the difference.
Minor: PlaywrightServiceConnector silently ignores BrowserTypeLaunchOptions in service mode
The options parameter is passed in from callers (who may have configured headless, slow-mo, etc.) but is never forwarded to ConnectAsync — BrowserTypeConnectOptions is used instead, which is correct for a remote session. However, the silent drop can confuse callers who do configure launch options. A one-line comment would help:
// BrowserTypeLaunchOptions are local-only; ConnectAsync uses BrowserTypeConnectOptions.
return await browserType.ConnectAsync(wsEndpoint, connectOptions).ConfigureAwait(false);Minor: missing newline at end of Directory.Build.props
The diff shows \ No newline at end of file. Trivial, but consistent with the rest of the file.
Strengths
The refactoring is clean and well-scoped. PlaywrightServiceConnector and PlaywrightTelemetryHeaders are good internal extractions — they eliminate duplication without leaking implementation detail. The TwoContextFixtureTests example is the right kind of smoke test: it verifies the semantic contract (isolated storage, shared browser reference) rather than just checking that types instantiate. The Directory.Build.props CI fix correctly mirrors the Aspire pattern.
The ContextFixture.GetContextOptions delta is the only material concern; everything else is minor polish.
- Update PublicAPI .verified.txt snapshots for the 4 new public fixture types (PlaywrightFixture, BrowserFixture, ContextFixture, PageFixture) across all four TFMs. - Add comment in PlaywrightServiceConnector explaining why BrowserTypeLaunchOptions are not forwarded to ConnectAsync (local-only; remote connect uses BrowserTypeConnectOptions). - Add trailing newline to Directory.Build.props.
|
Round 2 follow-ups: New issue ( public virtual BrowserNewContextOptions ContextOptions(TestContext testContext)
{
return new() { Locale = "en-US", ColorScheme = ColorScheme.Light, };
}
Minor (launch-options forwarding comment) — added one-liner in Minor (trailing newline on Also pushed: |
There was a problem hiding this comment.
Follow-up Review — Round 3
Previous findings: status check
Round 1 / Issue 1 (null! contradiction) — Deferred to a follow-up alignment PR. No change. ✓ Accepted.
Round 1 / Issue 2 (hardcoded scope docs) — Doc comments added to BrowserFixture and ContextFixture. ✓ Confirmed.
Round 2 / Minor (PageFixture default scope) — XML doc on PageFixture is present and accurate. ✓ Confirmed.
Round 2 / Minor (PlaywrightServiceConnector silent options drop) — Inline comment // BrowserTypeLaunchOptions are local-process only; remote connect uses BrowserTypeConnectOptions. added. ✓ Confirmed.
Round 2 / Minor (Directory.Build.props newline) — Newline added. ✓ Confirmed.
Remaining open finding: ContextFixture.GetContextOptions still diverges silently from BrowserTest
ContextFixture.cs still has:
protected virtual BrowserNewContextOptions GetContextOptions() =>
new() { Locale = "en-US", ColorScheme = ColorScheme.Light };No XML doc comment was added to this method to explain the divergence from BrowserTest, which leaves Locale and ColorScheme at the Playwright default. This was the primary finding in round 2 and remains unaddressed.
The method itself is protected virtual, so users can override it, but there is no indication that the base defaults are opinionated or that they differ from the inheritance-based path. A migration from ContextTest : BrowserTest to [ClassDataSource<ContextFixture>] will produce different context behaviour without any hint in the API.
Minimal fix (no behaviour change required):
/// <summary>
/// Returns the options used when creating each <see cref="IBrowserContext"/>.
/// The default pins <c>Locale</c> to <c>en-US</c> and <c>ColorScheme</c> to <c>Light</c>
/// for deterministic cross-platform rendering — this differs from the
/// <see cref="BrowserTest.NewContext"/> path, which uses the browser's default locale
/// and colour scheme. Override to match your application's locale requirements or to
/// restore browser-default behaviour.
/// </summary>
protected virtual BrowserNewContextOptions GetContextOptions() =>
new() { Locale = "en-US", ColorScheme = ColorScheme.Light };This is the only remaining material concern. Everything else in the PR is clean and well-implemented.
Summary
The PR is in good shape overall. The composition-based fixture API is a solid addition, the shared-helper extractions eliminate meaningful duplication, and the smoke test gives a clear runnable specification of the scoping contract. One XML doc comment on GetContextOptions would close the last outstanding review finding.
…ure defaults The local-build ProjectReference swap broke `dotnet run` on the template TestProject with "test framework adapter has not been registered". When NuGet resolves TUnit.Playwright as a package, build/TUnit.props is auto- imported and sets IsTestingPlatformApplication=true plus the TestingPlatformBuilderHook that registers TUnit. ProjectReference does not propagate those build assets, so explicitly import TUnit.props in the swap block. Also adds an XML doc comment on ContextFixture.GetContextOptions explaining that the en-US/Light defaults match ContextTest.ContextOptions (the inheritance counterpart) for migration parity, and how to override back to browser defaults.
|
Round 3 follow-ups in commit `d134b06`:
Separately — fixed a real CI regression caused by my own swap. The earlier |
There was a problem hiding this comment.
Follow-up Review — Round 4 (Final)
Previous findings: status check
Round 1 / Issue 1 (null! contradiction) — Deferred to a follow-up alignment PR. No change. ✓ Accepted.
Round 1 / Issue 2 (hardcoded scope docs) — Doc comments on BrowserFixture and ContextFixture present. ✓ Confirmed.
Round 1 / Minor (PageFixture default scope) — XML doc explaining SharedType.None is intentional is present and accurate. ✓ Confirmed.
Round 2 / Minor (PlaywrightServiceConnector silent options drop) — Inline comment present. ✓ Confirmed.
Round 2 / Minor (Directory.Build.props newline) — Fixed. ✓ Confirmed.
Round 2→3 / Main finding (ContextFixture.GetContextOptions defaults) — Verified in d134b06. Doc comment is now present and accurate:
/// <summary>
/// Returns the options used when creating each <see cref="IBrowserContext"/>. Defaults
/// match <see cref="ContextTest.ContextOptions"/> — pinned <c>Locale = "en-US"</c> and
/// <c>ColorScheme = Light</c> for deterministic cross-platform rendering. Override to
/// match your application's locale or to restore browser-default behaviour
/// (<c>new BrowserNewContextOptions()</c>).
/// </summary>This correctly documents parity with ContextTest.ContextOptions (not BrowserTest, as my round-2 review incorrectly compared against), and clearly explains both the rationale and the override escape hatch. ✓ Confirmed and closed.
No new findings
The PR is clean at d134b06. All four fixture classes are well-documented, the shared-helper extraction is correct, and the TwoContextFixtureTests smoke test gives a clear runnable specification of the scoping contract. The Directory.Build.props CI fix mirrors the established Aspire pattern and correctly handles the ProjectReference-doesn't-propagate-build-assets edge case.
This PR is good to merge.
1 participant
Summary
Adds an additive composition-based fixture API to
TUnit.Playwrightas an alternative to the existingPlaywrightTest/BrowserTest/ContextTest/PageTestinheritance hierarchy. Users who already have their own base classes — or who want fine-grained control over which Playwright objects are shared at which scope — can now compose via[ClassDataSource<T>]instead of inheriting.Each fixture implements
IAsyncInitializer/IAsyncDisposableand references the next layer via a property attributed with[ClassDataSource<T>(Shared = …)]:PlaywrightFixture— ownsIPlaywrightBrowserFixture—[ClassDataSource<PlaywrightFixture>(PerTestSession)], ownsIBrowser, supports the Microsoft Playwright Service env-var contractContextFixture—[ClassDataSource<BrowserFixture>(PerTestSession)], ownsIBrowserContext, performs W3C trace-header propagationPageFixture—[ClassDataSource<ContextFixture>], ownsIPageMulti-context use case
Two
[ClassDataSource<PageFixture>]properties yield two distinctIBrowserContextinstances (cookie / storage isolation) while sharing oneIBrowser— useful for testing two accounts interacting:Refactor
To avoid drift, two pieces of integration logic that previously lived only on the inheritance side were lifted to internal helpers and shared with both surfaces:
PlaywrightServiceConnector.LaunchAsync— used by bothBrowserService.CreateBrowserandBrowserFixture.InitializeAsyncPlaywrightTelemetryHeaders.Merge— used by bothBrowserTest.NewContextandContextFixture.InitializeAsyncThe existing
PlaywrightTest/BrowserTest/ContextTest/PageTestclasses are untouched in their public surface.Test plan
dotnet build TUnit.Playwright/TUnit.Playwright.csprojclean acrossnetstandard2.0;net8.0;net9.0;net10.0TwoContextFixtureTests.cs): twoPageFixtureinjections; setslocalStoragein Alice, asserts Bob'slocalStorageis empty; assertsAlice.Page != Bob.Page,Alice.ContextFixture.Context != Bob.ContextFixture.Context,Alice.…Browser == Bob.…BrowserTUnit.Templates.Tests/Snapshots/.../TUnit.Playwright/keepsPlaywriteTemplateTests.InstantiationTestgreenRunPlaywrightTestsModule(afterInstallPlaywrightModule) — exercises the smoke test end-to-end against real Chromium