Skip to content

[build] keep pre-installed browsers on Windows, delete only drivers#17650

Merged
titusfortner merged 2 commits into
trunkfrom
win_gh_browsers
Jun 6, 2026
Merged

[build] keep pre-installed browsers on Windows, delete only drivers#17650
titusfortner merged 2 commits into
trunkfrom
win_gh_browsers

Conversation

@titusfortner
Copy link
Copy Markdown
Member

Seeing some failures where the Windows deletion script results in a corrupted Firefox which breaks Selenium Manager.

💥 What does this PR do?

Deleting browsers on Windows isn't as reliable, so this changes the script to only delete the drivers. This
still exercises Selenium Manager, just using a different path; use system browsers and download correct drivers

🤖 AI assistance

  • AI assisted (complete below)
    • Tool(s): Claude Code
    • What was generated: the script/workflow changes
    • I reviewed all AI output and can explain the change

🔄 Types of changes

  • Bug fix (backwards compatible)

@selenium-ci selenium-ci added the B-build Includes scripting, bazel and CI integrations label Jun 6, 2026
@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Keep pre-installed browsers on Windows, delete only drivers

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Keep pre-installed browsers on Windows, delete only drivers
• Prevents corrupted Firefox installations from breaking Selenium Manager
• Windows browser deletion is slow and unreliable, leaving partial installs
• Selenium Manager still exercises driver download path with system browsers
Diagram
flowchart LR
  A["Windows CI Environment"] -->|Previously| B["Delete browsers and drivers"]
  B -->|Result| C["Corrupted Firefox breaks Selenium Manager"]
  A -->|Now| D["Delete only drivers"]
  D -->|Result| E["Selenium Manager downloads clean drivers"]
  E -->|Uses| F["Pre-installed system browsers"]

Loading

Grey Divider

File Changes

1. scripts/github-actions/delete-browsers-drivers.ps1 🐞 Bug fix +10/-7

Remove browser deletion, keep driver cleanup

• Removed deletion of pre-installed browsers (Chrome, Firefox, Edge)
• Kept deletion of WebDriver binaries only
• Updated script comments to explain Windows-specific reliability issues
• Simplified paths array to exclude browser installation directories

scripts/github-actions/delete-browsers-drivers.ps1


2. .github/workflows/bazel.yml ⚙️ Configuration changes +1/-1

Update Windows workflow step name

• Updated workflow step name from "Delete browsers and drivers (Windows)" to "Delete drivers
 (Windows)"
• Reflects the change in script behavior to only delete drivers

.github/workflows/bazel.yml


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jun 6, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX issues (0)

Grey Divider


Action required

1. Empty driver path crash ✓ Resolved 🐞 Bug ☼ Reliability
Description
delete-browsers-drivers.ps1 now builds $paths only from WebDriver env vars and calls Remove-Item
unconditionally. If none of those env vars are set on the Windows runner, $paths is empty and
PowerShell parameter binding fails before -ErrorAction applies, breaking the workflow step and
leaving preinstalled drivers in place.
Code

scripts/github-actions/delete-browsers-drivers.ps1[R14-20]

$paths = @(
  $env:ChromeWebDriver,
  $env:EdgeWebDriver,
-  $env:GeckoWebDriver,
-  "C:\Program Files\Google\Chrome",
-  "C:\Program Files\Mozilla Firefox",
-  "C:\Program Files (x86)\Microsoft\Edge"
+  $env:GeckoWebDriver
) | Where-Object { $_ }

Remove-Item -Path $paths -Recurse -Force -ErrorAction SilentlyContinue
Evidence
The Windows workflow step runs the script without defining any driver env vars, while the script
filters paths solely from env vars and then calls Remove-Item on the resulting (possibly empty)
array. The Linux/macOS script explicitly documents env-var-based driver discovery for those OSes,
highlighting that Windows may not provide the same guarantees.

.github/workflows/bazel.yml[202-208]
scripts/github-actions/delete-browsers-drivers.ps1[12-20]
scripts/github-actions/delete-browsers-drivers.sh[9-14]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
On Windows, `Remove-Item -Path $paths ...` is called even when `$paths` is empty (because it’s filtered from env vars only). PowerShell throws a parameter-binding error for an empty collection before `-ErrorAction SilentlyContinue` can suppress anything, causing the step to fail.

## Issue Context
The workflow does not set these env vars explicitly for Windows; the step relies on the runner to provide them.

## Fix Focus Areas
- scripts/github-actions/delete-browsers-drivers.ps1[12-20]
- .github/workflows/bazel.yml[202-208]

### Suggested fix approach
- Add a guard:
 - If `$paths.Count -eq 0`, print a message and `exit 0` (or discover driver locations via `Get-Command chromedriver/msedgedriver/geckodriver` and delete those paths).
- Optionally: set these env vars in the workflow step (if known stable locations exist), or add fallback known locations used by GitHub Windows runners.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Remove-Item errors silenced 📘 Rule violation ☼ Reliability
Description
The Windows cleanup script suppresses deletion failures via -ErrorAction SilentlyContinue, which
can cause the workflow to proceed with pre-installed drivers still present and no actionable signal.
CI automation should validate outcomes and fail safely (or at least emit explicit warnings) when
expected cleanup cannot be performed.
Code

scripts/github-actions/delete-browsers-drivers.ps1[R15-16]

+if ($paths) {
+  Remove-Item -Path $paths -Recurse -Force -ErrorAction SilentlyContinue
Evidence
Rule 10 requires CI/workflow automation to validate outputs and fail safely rather than ignoring
errors. The added block still calls Remove-Item with -ErrorAction SilentlyContinue, which
suppresses real deletion failures and can produce silent/unsafe success.

scripts/github-actions/delete-browsers-drivers.ps1[15-16]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The script uses `Remove-Item ... -ErrorAction SilentlyContinue`, which can hide failures to delete existing driver paths and let CI continue in an unexpected state.

## Issue Context
This is CI/workflow automation; per compliance guidance it should validate external effects and fail safely (or emit explicit warnings) rather than silently ignoring errors.

## Fix Focus Areas
- scripts/github-actions/delete-browsers-drivers.ps1[15-16]

## Implementation notes
- Consider iterating each path and using `Test-Path` to distinguish “missing path” (ok) from “delete failed” (warn or fail).
- Prefer `-ErrorAction Stop` with `try/catch` that `Write-Error` (and `exit 1`) or at minimum `Write-Warning` when deletion fails for an existing path.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Advisory comments

3. Misleading script filename ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The Windows step and script comments now indicate only drivers are deleted, but the script filename
remains delete-browsers-drivers.ps1. This mismatch can mislead future changes/reviews into assuming
browsers are still deleted on Windows.
Code

.github/workflows/bazel.yml[R205-208]

+      - name: Delete drivers (Windows)
        if: inputs.os == 'windows'
        shell: pwsh
        run: ./scripts/github-actions/delete-browsers-drivers.ps1
Evidence
The workflow step name was updated to drivers-only, but it still calls a script whose filename
implies it deletes both browsers and drivers, while the script itself no longer includes browser
paths.

.github/workflows/bazel.yml[202-208]
scripts/github-actions/delete-browsers-drivers.ps1[1-20]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The Windows deletion behavior was narrowed to drivers-only, but the script name still suggests it deletes browsers too.

## Issue Context
This is a maintainability/clarity issue (not functional) that could cause confusion later.

## Fix Focus Areas
- .github/workflows/bazel.yml[205-208]
- scripts/github-actions/delete-browsers-drivers.ps1[1-20]

### Suggested fix approach
- Rename the script to `delete-drivers.ps1` (or similar) and update the workflow invocation accordingly; or
- Keep the filename but add a short comment in the workflow step noting that the script is drivers-only on Windows.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread scripts/github-actions/delete-browsers-drivers.ps1 Outdated
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jun 6, 2026

Code review by qodo was updated up to the latest commit 896f53c

@titusfortner titusfortner merged commit 5f14f8d into trunk Jun 6, 2026
26 checks passed
@titusfortner titusfortner deleted the win_gh_browsers branch June 6, 2026 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants