Move Playwright install into pipeline module

[thomhurst]

Summary

• Adds InstallPlaywrightModule to TUnit.Pipeline which runs npx playwright install --with-deps via bash
• RunPlaywrightTestsModule gains [DependsOn<InstallPlaywrightModule>] so it waits for installation before running tests
• Removes the Playwright cache and install steps from dotnet.yml — installation now happens concurrently with other pipeline work (build, unit tests, etc.) rather than blocking upfront

Test Plan

• Verify CI pipeline runs Playwright tests successfully
• Confirm InstallPlaywrightModule completes before RunPlaywrightTestsModule starts

[claude]

Code Review

The PR achieves its stated goal of moving Playwright installation into the pipeline module system, which is a good architectural direction. However, there are a few issues worth addressing before merging.

⚠️ Loss of Browser Caching

The most impactful concern: the original CI workflow had an actions/cache@v5 step that cached the Playwright browser binaries between runs. That cache is now removed entirely, and InstallPlaywrightModule unconditionally runs npx playwright install --with-deps on every CI run.

Playwright browser downloads are large (~200–400MB per browser per OS). This will meaningfully increase CI time on every run compared to the previous cache-hit path. If caching isn't feasible inside the pipeline module, consider at minimum keeping the actions/cache step in the YAML (without the install steps), and checking for cache hits inside the module before running the install command.

⚠️ Unintentional Removal of RunOnLinuxOnly, RunOnWindowsOnly

The diff removes these two attributes from RunPlaywrightTestsModule:

-[NotInParallel("NetworkTests"), RunOnLinuxOnly, RunOnWindowsOnly]
+[NotInParallel("NetworkTests")]
+[DependsOn<InstallPlaywrightModule>]

Looking at sibling modules, this pattern is intentional — it means "skip on macOS":

• RunRpcTestsModule: [RunOnLinuxOnly, RunOnWindowsOnly]
• RunAspireTestsModule: [RunOnLinuxOnly, RunOnWindowsOnly]
• RunAspNetTestsModule: [RunOnLinuxOnly, RunOnWindowsOnly]

Removing these attributes means Playwright tests (and InstallPlaywrightModule) will now also run on macos-latest. Was that intentional? If not, these attributes should be restored — either on RunPlaywrightTestsModule directly, or also on InstallPlaywrightModule to avoid installing browsers on macOS unnecessarily.

ℹ️ Bash Shell on Windows

InstallPlaywrightModule uses context.Shell.Bash.Command(new BashCommandOptions(...)), which explicitly invokes bash. The original CI step used the runner's default shell (pwsh on Windows). GitHub Actions Windows runners have Git Bash available, so this will likely work in practice, but it's a subtle behavioral difference worth being aware of if this module is ever used outside the CI context.

✅ What's Good

• Moving the install step into a typed module is the right direction — it keeps infrastructure logic close to the code that needs it and makes the dependency explicit via [DependsOn<InstallPlaywrightModule>].
• Removing 21 lines from the YAML is a real simplification.
• The module implementation itself is clean and minimal.

Summary: The caching regression and the RunOnLinuxOnly, RunOnWindowsOnly removal are the two items that should be addressed before merging.

[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

_{TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Follow-up Review

The second commit ("Restore Playwright browser caching and conditional install") addressed the primary caching concern from my previous review — the actions/cache@v5 step is back in the YAML and InstallPlaywrightModule now checks for local cache presence via IsBrowserCachePresent() before deciding whether to run --with-deps. Good fix.

⚠️ Still Unresolved: Missing RunOnLinuxOnly, RunOnWindowsOnly

The removal of these attributes from RunPlaywrightTestsModule has not been addressed, and now InstallPlaywrightModule also lacks them. Since the CI matrix includes macos-latest, Playwright tests (and browser installation) will now run on macOS as well.

Every comparable module in the pipeline restricts to Linux + Windows:

• RunAspNetTestsModule: [RunOnLinuxOnly, RunOnWindowsOnly]
• RunAspireTestsModule: [RunOnLinuxOnly, RunOnWindowsOnly]
• RunRpcTestsModule: [RunOnLinuxOnly, RunOnWindowsOnly]

If running Playwright on macOS is intentional, the PR description should say so. If not, both modules need the attributes restored:

// InstallPlaywrightModule.cs
[RunOnLinuxOnly, RunOnWindowsOnly]
public class InstallPlaywrightModule : Module<CommandResult>

// RunPlaywrightTestsModule.cs
[NotInParallel("NetworkTests"), RunOnLinuxOnly, RunOnWindowsOnly]
[DependsOn<InstallPlaywrightModule>]
public class RunPlaywrightTestsModule : TestBaseModule

ℹ️ Cache Presence Check is Shallow

IsBrowserCachePresent() checks only whether any subdirectory exists in the cache path. A partial or corrupt cache restore (which continue-on-error: true in the YAML allows) would pass this check but produce an incomplete install, leading to test failures rather than a clean re-install. A more defensive version would look for a known-good browser directory (e.g. chromium-*) or simply always run npx playwright install and rely on Playwright's own idempotency checks — Playwright already skips downloads when browsers are current.

ℹ️ Bash Shell on Windows (Unchanged)

context.Shell.Bash.Command(...) still uses bash explicitly. Git Bash is available on GitHub Actions Windows runners so this works in practice, but it differs from the original CI step which used the runner default shell (pwsh). No change needed unless this module is reused outside CI.

✅ What's Good

• The caching regression from the first commit is now fixed.
• The two-commit approach (introduce feature → restore caching) is clear in git history.
• Module logic itself is clean.

The RunOnLinuxOnly, RunOnWindowsOnly question is the only blocking item — either confirm macOS Playwright runs are intentional, or restore the attributes.

[claude]

Review

This is a clean, well-scoped change that correctly moves Playwright installation into the pipeline so it can run concurrently with other work (build, unit tests, etc.) rather than blocking the entire CI job upfront. The intent and structure are sound. A few things worth noting:

Bug: [RunOnLinuxOnly, RunOnWindowsOnly] removed from RunPlaywrightTestsModule

The original module had [NotInParallel("NetworkTests"), RunOnLinuxOnly, RunOnWindowsOnly]. The PR drops the two platform attributes but does not add them to InstallPlaywrightModule. This means on macOS runners (if ever added), both modules would now execute, whereas previously they were skipped. More importantly, InstallPlaywrightModule uses context.Shell.Bash.Command(...) which is only available on Unix-like systems. On Windows CI runners, bash may not be in PATH in the same way (or Playwright's --with-deps installs Linux apt packages that don't apply).

If the intent is that Playwright tests only run on Linux and Windows (not macOS), the platform constraints should be preserved — either on InstallPlaywrightModule itself, or by restoring the attributes on RunPlaywrightTestsModule. Looking at the analogous RunAspNetTestsModule and RunAspireTestsModule, they keep [RunOnLinuxOnly, RunOnWindowsOnly] — this pattern should be consistent.

Observation: InstallPlaywrightModule always installs --with-deps

The original YAML distinguished between a cache hit (npx playwright install, browsers only) and a miss (npx playwright install --with-deps, browsers + system deps). The new module always runs --with-deps. On Linux this installs OS-level dependencies via apt, which takes extra time even when browsers are cached. This is intentional if simplicity is preferred over performance, but worth documenting or considering whether the module should probe the cache state to mirror the original behavior.

Minor: context.Shell.Bash cross-platform assumptions

Shell.Bash.Command works on Linux/macOS. On Windows it invokes Git Bash or WSL depending on the ModularPipelines version. Since InstallPlaywrightModule has no platform guard, this could silently fail or do the wrong thing on Windows. The existing PublishAndRunMockTestsAOTModule avoids this by explicitly gating on RuntimeInformation.IsOSPlatform(OSPlatform.Linux) via ModuleConfiguration.Create().WithSkipWhen(...). Consider adding a similar guard or restoring the attribute:

[RunOnLinuxOnly, RunOnWindowsOnly]
public class InstallPlaywrightModule : Module<CommandResult>

or using a Configure() override for finer control.

The concurrency improvement is real

The core idea is good — moving install into the pipeline's dependency graph means it overlaps with build/pack/other test steps instead of being a serial gate in YAML. The [DependsOn<InstallPlaywrightModule>] dependency correctly enforces ordering without sacrificing the parallelism gain.

Summary

The main concern is the missing platform constraint on InstallPlaywrightModule. If RunPlaywrightTestsModule was previously restricted to Linux and Windows only, that constraint needs to travel to InstallPlaywrightModule as well (since it will be executed because of the dependency chain). Everything else is clean and the approach is architecturally sound.

[claude]

Final Review — Remaining Unresolved Issue

This PR has been reviewed three times now. Two concerns have been satisfactorily addressed (browser caching regression, working directory fix). One has not been addressed across any of the four commits:

Still Unresolved: Missing Platform Guards on Both Modules

The current state of the merged code:

// InstallPlaywrightModule.cs — no platform attributes
public class InstallPlaywrightModule : Module<CommandResult>

// RunPlaywrightTestsModule.cs — platform attributes removed
[NotInParallel("NetworkTests")]
[DependsOn<InstallPlaywrightModule>]
public class RunPlaywrightTestsModule : TestBaseModule

The previous state:

[NotInParallel("NetworkTests"), RunOnLinuxOnly, RunOnWindowsOnly]
public class RunPlaywrightTestsModule : TestBaseModule

Every comparable module in the pipeline applies [RunOnLinuxOnly, RunOnWindowsOnly]:

• RunAspNetTestsModule (line 13)
• RunAspireTestsModule
• RunRpcTestsModule

Why this matters:

1. InstallPlaywrightModule calls context.Shell.Bash.Command(...). On macOS CI runners (if ever added), bash exists but npx playwright install --with-deps installs Linux system packages via apt — this will fail or partially succeed silently on macOS.
2. If the CI matrix ever expands to include macos-latest, both modules will now execute on macOS when they previously did not.
3. The [DependsOn<InstallPlaywrightModule>] means that wherever RunPlaywrightTestsModule runs, InstallPlaywrightModule also runs — so InstallPlaywrightModule must carry the same platform constraints as the tests it supports.

Fix:

// InstallPlaywrightModule.cs
[RunOnLinuxOnly, RunOnWindowsOnly]
public class InstallPlaywrightModule : Module<CommandResult>

// RunPlaywrightTestsModule.cs
[NotInParallel("NetworkTests"), RunOnLinuxOnly, RunOnWindowsOnly]
[DependsOn<InstallPlaywrightModule>]
public class RunPlaywrightTestsModule : TestBaseModule

If running Playwright on macOS is intentionally new behavior introduced by this PR, please update the PR description to say so — as it stands there is no mention of it, and it breaks the pattern established by all sibling modules.

---

The rest of the PR (concurrent install via [DependsOn], simplified YAML, correct working directory) is good. This is the only item blocking a clean merge.

[claude]

Fourth Review — Same Blocking Issue Remains Unresolved

This PR has now accumulated 5 commits and 3 prior review cycles. The working directory fix and the browser caching regression have both been properly addressed. However, the single blocking issue flagged in every prior review has still not been addressed.

Still Missing: Platform Guards on Both Modules

Current state of the PR head:

// InstallPlaywrightModule.cs — no platform attributes
public class InstallPlaywrightModule : Module<CommandResult>

// RunPlaywrightTestsModule.cs — [RunOnLinuxOnly, RunOnWindowsOnly] removed, never restored
[NotInParallel("NetworkTests")]
[DependsOn<InstallPlaywrightModule>]
public class RunPlaywrightTestsModule : TestBaseModule

Every comparable network-test module in the pipeline carries [RunOnLinuxOnly, RunOnWindowsOnly]:

• RunAspNetTestsModule — [NotInParallel("NetworkTests"), RunOnLinuxOnly, RunOnWindowsOnly]
• RunAspireTestsModule
• RunRpcTestsModule

RunPlaywrightTestsModule had these attributes before this PR. They were removed and never restored.

Why this matters:

1. InstallPlaywrightModule uses context.Shell.Bash.Command(...) — on Windows this delegates to Git Bash/WSL and npx playwright install --with-deps installs Linux apt packages that don't apply on Windows. This is an existing runtime risk.
2. If the CI matrix ever expands to macos-latest, both modules will execute on macOS when they previously did not. --with-deps on macOS will fail because it tries to invoke apt/dpkg.
3. The [DependsOn<InstallPlaywrightModule>] coupling means wherever RunPlaywrightTestsModule runs, InstallPlaywrightModule also runs — so the install module must carry the same platform constraints.

Required fix (two lines):

// InstallPlaywrightModule.cs
[RunOnLinuxOnly, RunOnWindowsOnly]
public class InstallPlaywrightModule : Module<CommandResult>

// RunPlaywrightTestsModule.cs
[NotInParallel("NetworkTests"), RunOnLinuxOnly, RunOnWindowsOnly]
[DependsOn<InstallPlaywrightModule>]
public class RunPlaywrightTestsModule : TestBaseModule

---

The structural improvement in this PR (moving installation into the pipeline so it runs concurrently with build/pack/tests) is genuinely good. This is a two-line fix. Everything else looks correct and clean.