Skip to content

[build] allow ruby and python to run remote tests on windows#17603

Merged
titusfortner merged 1 commit into
trunkfrom
c/jdk_realpath
Jun 1, 2026
Merged

[build] allow ruby and python to run remote tests on windows#17603
titusfortner merged 1 commit into
trunkfrom
c/jdk_realpath

Conversation

@titusfortner
Copy link
Copy Markdown
Member

@titusfortner titusfortner commented May 31, 2026

Situation

I noticed that Ruby can't run remote tests on Windows right now (server crashes) and I tracked it down to a real JDK symlink bug

  • Java avoids this by running Grid tests in the same process
  • Ruby and Python are currently skipping running Grid tests on Windows
  • .NET runs Grid tests on Windows using System Java not Bazel Java (to be addressed later)
  • JS doesn't run any tests on Windows

💥 What does this PR do?

Creates Ruby & Python workarounds to get the JDK home, resolve it via runfiles then get the real path of Java on Windows to avoid the symlink.

🔧 Implementation Notes

I didn't want to add new Windows Remote tests with this PR, but to show that the new code works: https://github.com/SeleniumHQ/selenium/actions/runs/26725902074/job/78761168501

Considered:

  • Turning off symlinks - dramatically balloons the disk space to put everything in every directory
  • Pointing at a non-Bazel JAVA_HOME on Windows (what .NET bindings do now) breaks hermeticity.
  • Fixing the genrule at build time to emit a non-symlinked path — not possible; the symlink only exists in the per-test runfiles tree at runtime.

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s): Claude Code
    • What was generated: `
    • I reviewed all AI output and can explain the change

💡 Additional Considerations

  • .NET should add a new genrule to use hermetic java for tests

🔄 Types of changes

  • Bug fix (backwards compatible)

@selenium-ci selenium-ci added C-py Python Bindings C-rb Ruby Bindings B-build Includes scripting, bazel and CI integrations labels May 31, 2026
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 31, 2026

Review Summary by Qodo

(Agentic_describe updated until commit 4fb048c)

Resolve JDK symlink paths for Windows remote tests

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Resolve JDK symlink path on Windows for Ruby and Python
• Use os.path.realpath() in Python to resolve symlink
• Use File.realpath() in Ruby to resolve symlink
• Add platform check to apply fix only on Windows
Diagram
flowchart LR
  A["JDK Path from Runfiles"] --> B["Resolve via rlocation"]
  B --> C{"Windows Platform?"}
  C -->|Yes| D["Apply realpath Resolution"]
  C -->|No| E["Use Path As-Is"]
  D --> F["Java Path Ready"]
  E --> F

Loading

Grey Divider

File Changes

1. py/conftest.py 🐞 Bug fix +7/-3

Add Windows JDK symlink resolution for Python

• Check for SE_BAZEL_JAVA_LOCATION environment variable before processing
• Apply os.path.realpath() to Java path on Windows platform
• Verify path exists before applying realpath resolution

py/conftest.py


2. rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb 🐞 Bug fix +2/-1

Add Windows JDK symlink resolution for Ruby

• Store resolved path in intermediate variable
• Apply File.realpath() on Windows platform only
• Conditionally resolve symlink based on platform and file existence

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 31, 2026

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (2)

Grey Divider


Action required

1. Missing env var crashes 🐞 Bug ≡ Correctness
Description
In py/conftest.py, the remote server fixture concatenates a string with
os.environ.get("SE_BAZEL_JAVA_HOME") without checking for None, so an unset env var raises TypeError
before the try/except and aborts the session.
Code

py/conftest.py[569]

Evidence
The fixture concatenates a string with a possibly-missing env var outside the try/except, which
deterministically raises TypeError when the env var is absent.

py/conftest.py[566-572]

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

### Issue description
`server()` builds a runfiles path via `"_main/" + os.environ.get("SE_BAZEL_JAVA_HOME")`. If the env var is unset, `os.environ.get(...)` returns `None` and Python raises `TypeError` during string concatenation. This happens **before** the `try:` block, so the exception is not caught and the whole test session fails.

### Issue Context
This code runs only for `--remote` (Grid) sessions. It is expected to work under Bazel, but it should also fail gracefully (or at least fail with a clear error) if the env var is missing.

### Fix Focus Areas
- py/conftest.py[566-582]

### Suggested change
- Read the env var into a local variable and guard it:
 - If unset: either (a) skip setting `server.java_path` (leaving default behavior) or (b) raise a clear error like `RuntimeError("SE_BAZEL_JAVA_HOME not set; cannot locate Bazel JDK")`.
- Ensure the `Rlocation(...)` call is only executed when you have a non-empty env var value.

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



Remediation recommended

2. SE_BAZEL_JAVA_LOCATION renamed 📘 Rule violation ⚙ Maintainability
Description
The PR replaces SE_BAZEL_JAVA_LOCATION with SE_BAZEL_JAVA_HOME without any backward-compatible
fallback, which can break existing test runners or scripts that still set the old variable. This
risks breaking consumers of this user-facing configuration surface.
Code

py/conftest.py[R569-572]

Evidence
PR Compliance ID 1 requires avoiding breaking changes to public/user-facing interfaces. The updated
Bazel test target sets only SE_BAZEL_JAVA_HOME, and the Python test harness reads only
SE_BAZEL_JAVA_HOME with no compatibility fallback for the previous SE_BAZEL_JAVA_LOCATION name.

AGENTS.md: Maintain API/ABI Compatibility for Public Interfaces
py/BUILD.bazel[814-821]
py/conftest.py[566-580]

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

## Issue description
`py/conftest.py` now reads only `SE_BAZEL_JAVA_HOME`, but older invocations may still provide `SE_BAZEL_JAVA_LOCATION`. This creates a breaking change in configuration/API surface.

## Issue Context
The Bazel test rule now sets `SE_BAZEL_JAVA_HOME`, but external runners or pinned tooling may still set the old variable name.

## Fix Focus Areas
- py/conftest.py[566-580]
- py/BUILD.bazel[814-821]

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


3. WD_BAZEL_JAVA_LOCATION renamed 📘 Rule violation ⚙ Maintainability
Description
The PR replaces WD_BAZEL_JAVA_LOCATION with WD_BAZEL_JAVA_HOME without a backward-compatible
fallback, which can break existing scripts or Bazel invocations relying on the old variable name.
This is a potential breaking change for consumers of the Ruby test environment interface.
Code

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[R137-140]

Evidence
PR Compliance ID 1 requires maintaining compatibility for public/user-facing interfaces. The Bazel
rule now exports only WD_BAZEL_JAVA_HOME, and the Ruby helper checks only for WD_BAZEL_JAVA_HOME
(no fallback to the old variable), which can break existing consumers still using
WD_BAZEL_JAVA_LOCATION.

AGENTS.md: Maintain API/ABI Compatibility for Public Interfaces
rb/spec/tests.bzl[222-232]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[136-149]

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 Ruby test environment now requires `WD_BAZEL_JAVA_HOME` and no longer accepts `WD_BAZEL_JAVA_LOCATION`, which can break existing invocations.

## Issue Context
`rb/spec/tests.bzl` sets the new variable name, but external consumers may still set the old one.

## Fix Focus Areas
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[136-149]
- rb/spec/tests.bzl[222-232]

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


4. Silent system Java fallback 🐞 Bug ☼ Reliability
Description
If resolving the Bazel JDK home fails in py/conftest.py, the code swallows the exception and leaves
Server.java_path unset; Server.start() then uses shutil.which("java"), which can bypass the intended
hermetic JDK and Windows realpath workaround.
Code

py/conftest.py[R570-582]

Evidence
py/conftest.py suppresses all exceptions during java path setup, and the Server implementation
explicitly falls back to PATH Java when java_path is unset.

py/conftest.py[554-582]
py/selenium/webdriver/remote/server.py[172-183]

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 Java-home resolution block catches `Exception` and does nothing. If `java_home_txt` can't be located/read, `Rlocation` returns None, or the constructed `bin/java(.exe)` path doesn't exist, the assignment to `server.java_path` is skipped. Downstream, `Server.start()` falls back to `shutil.which("java")`, which can select a system Java instead of the Bazel JDK.

### Issue Context
This PR’s goal is to reliably use Bazel’s JDK (and on Windows, its real path) for Grid tests. Silent fallback makes failures non-obvious and can reintroduce the original Windows crash if system Java points into the runfiles symlink farm.

### Fix Focus Areas
- py/conftest.py[570-582]

### Suggested change
- Narrow the exception handling and/or add explicit validation:
 - If `SE_BAZEL_JAVA_HOME` is set but `java_home_txt`/`java_home`/`server.java_path` cannot be resolved, raise a clear error (preferred for CI correctness), or at minimum log a warning with details.
 - Check `java_home_txt` and `java_home` for falsy values before proceeding.
 - Optionally assert that the final `server.java_path` exists before setting it (so failures are explicit).

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


Grey Divider

Previous review results

Review updated until commit 4fb048c

Results up to commit 88061f6


🐞 Bugs (2) 📘 Rule violations (2) 📎 Requirement gaps (0)


Action required
1. Missing env var crashes 🐞 Bug ≡ Correctness
Description
In py/conftest.py, the remote server fixture concatenates a string with
os.environ.get("SE_BAZEL_JAVA_HOME") without checking for None, so an unset env var raises TypeError
before the try/except and aborts the session.
Code

py/conftest.py[569]

Evidence
The fixture concatenates a string with a possibly-missing env var outside the try/except, which
deterministically raises TypeError when the env var is absent.

py/conftest.py[566-572]

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

### Issue description
`server()` builds a runfiles path via `"_main/" + os.environ.get("SE_BAZEL_JAVA_HOME")`. If the env var is unset, `os.environ.get(...)` returns `None` and Python raises `TypeError` during string concatenation. This happens **before** the `try:` block, so the exception is not caught and the whole test session fails.

### Issue Context
This code runs only for `--remote` (Grid) sessions. It is expected to work under Bazel, but it should also fail gracefully (or at least fail with a clear error) if the env var is missing.

### Fix Focus Areas
- py/conftest.py[566-582]

### Suggested change
- Read the env var into a local variable and guard it:
 - If unset: either (a) skip setting `server.java_path` (leaving default behavior) or (b) raise a clear error like `RuntimeError("SE_BAZEL_JAVA_HOME not set; cannot locate Bazel JDK")`.
- Ensure the `Rlocation(...)` call is only executed when you have a non-empty env var value.

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



Remediation recommended
2. SE_BAZEL_JAVA_LOCATION renamed 📘 Rule violation ⚙ Maintainability
Description
The PR replaces SE_BAZEL_JAVA_LOCATION with SE_BAZEL_JAVA_HOME without any backward-compatible
fallback, which can break existing test runners or scripts that still set the old variable. This
risks breaking consumers of this user-facing configuration surface.
Code

py/conftest.py[R569-572]

Evidence
PR Compliance ID 1 requires avoiding breaking changes to public/user-facing interfaces. The updated
Bazel test target sets only SE_BAZEL_JAVA_HOME, and the Python test harness reads only
SE_BAZEL_JAVA_HOME with no compatibility fallback for the previous SE_BAZEL_JAVA_LOCATION name.

AGENTS.md: Maintain API/ABI Compatibility for Public Interfaces
py/BUILD.bazel[814-821]
py/conftest.py[566-580]

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

## Issue description
`py/conftest.py` now reads only `SE_BAZEL_JAVA_HOME`, but older invocations may still provide `SE_BAZEL_JAVA_LOCATION`. This creates a breaking change in configuration/API surface.

## Issue Context
The Bazel test rule now sets `SE_BAZEL_JAVA_HOME`, but external runners or pinned tooling may still set the old variable name.

## Fix Focus Areas
- py/conftest.py[566-580]
- py/BUILD.bazel[814-821]

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


3. WD_BAZEL_JAVA_LOCATION renamed 📘 Rule violation ⚙ Maintainability
Description
The PR replaces WD_BAZEL_JAVA_LOCATION with WD_BAZEL_JAVA_HOME without a backward-compatible
fallback, which can break existing scripts or Bazel invocations relying on the old variable name.
This is a potential breaking change for consumers of the Ruby test environment interface.
Code

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[R137-140]

Evidence
PR Compliance ID 1 requires maintaining compatibility for public/user-facing interfaces. The Bazel
rule now exports only WD_BAZEL_JAVA_HOME, and the Ruby helper checks only for WD_BAZEL_JAVA_HOME
(no fallback to the old variable), which can break existing consumers still using
WD_BAZEL_JAVA_LOCATION.

AGENTS.md: Maintain API/ABI Compatibility for Public Interfaces
rb/spec/tests.bzl[222-232]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[136-149]

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 Ruby test environment now requires `WD_BAZEL_JAVA_HOME` and no longer accepts `WD_BAZEL_JAVA_LOCATION`, which can break existing invocations.

## Issue Context
`rb/spec/tests.bzl` sets the new variable name, but external consumers may still set the old one.

## Fix Focus Areas
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[136-149]
- rb/spec/tests.bzl[222-232]

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


4. Silent system Java fallback 🐞 Bug ☼ Reliability
Description
If resolving the Bazel JDK home fails in py/conftest.py, the code swallows the exception and leaves
Server.java_path unset; Server.start() then uses shutil.which("java"), which can bypass the intended
hermetic JDK and Windows realpath workaround.
Code

py/conftest.py[R570-582]

Evidence
py/conftest.py suppresses all exceptions during java path setup, and the Server implementation
explicitly falls back to PATH Java when java_path is unset.

py/conftest.py[554-582]
py/selenium/webdriver/remote/server.py[172-183]

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 Java-home resolution block catches `Exception` and does nothing. If `java_home_txt` can't be located/read, `Rlocation` returns None, or the constructed `bin/java(.exe)` path doesn't exist, the assignment to `server.java_path` is skipped. Downstream, `Server.start()` falls back to `shutil.which("java")`, which can select a system Java instead of the Bazel JDK.

### Issue Context
This PR’s goal is to reliably use Bazel’s JDK (and on Windows, its real path) for Grid tests. Silent fallback makes failures non-obvious and can reintroduce the original Windows crash if system Java points into the runfiles symlink farm.

### Fix Focus Areas
- py/conftest.py[570-582]

### Suggested change
- Narrow the exception handling and/or add explicit validation:
 - If `SE_BAZEL_JAVA_HOME` is set but `java_home_txt`/`java_home`/`server.java_path` cannot be resolved, raise a clear error (preferred for CI correctness), or at minimum log a warning with details.
 - Check `java_home_txt` and `java_home` for falsy values before proceeding.
 - Optionally assert that the final `server.java_path` exists before setting it (so failures are explicit).

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


Qodo Logo

Comment thread py/conftest.py Outdated
@titusfortner titusfortner marked this pull request as draft May 31, 2026 19:47
@titusfortner titusfortner changed the title [rb][py] resolve runfiles JDK realpath on Windows to sidestep lib\modules symlink bug [build] allow ruby and python to run remote tests on windows May 31, 2026
Comment thread .github/workflows/PR17603.yml Fixed
Comment thread .github/workflows/PR17603.yml Fixed
Comment thread .github/workflows/PR17603.yml Fixed
@titusfortner titusfortner force-pushed the c/jdk_realpath branch 2 times, most recently from 1cbf19f to 8d45498 Compare May 31, 2026 23:43
@titusfortner titusfortner marked this pull request as ready for review June 1, 2026 00:31
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jun 1, 2026

Code review by qodo was updated up to the latest commit 4fb048c

@titusfortner titusfortner merged commit b02c5f2 into trunk Jun 1, 2026
53 checks passed
@titusfortner titusfortner deleted the c/jdk_realpath branch June 1, 2026 02:21
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 C-py Python Bindings C-rb Ruby Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants