[build] Use target_compatible_with to gate IE/Safari tests

[titusfortner]

💥 What does this PR do?

We're using the skip-rbe tag to prevent Mac and Windows specific tests from running on RBE. We don't need that if we use target_compatible_with and specify the OS.

🔧 Implementation Notes

I also removed the Java skiptest check because it was being used by bazel to tell JUnit not to run based on OS, which it shouldn't need to do with this.

🤖 AI assistance

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

[qodo-code-review]

Review Summary by Qodo

Replace skip-rbe tags with target_compatible_with for OS-gated tests

✨ Enhancement 🧪 Tests

Walkthroughs

Description

• Replace skip-rbe tags with target_compatible_with for OS-specific tests
• Remove selenium.skiptest Java system property checks from test code
• Add explicit target_compatible_with constraints to IE/Safari browser configurations
• Simplify JVM flags by eliminating conditional skiptest logic

Diagram

flowchart LR
  A["skip-rbe tags"] -->|"replaced by"| B["target_compatible_with constraints"]
  C["selenium.skiptest property"] -->|"removed from"| D["Java test code"]
  E["Conditional JVM flags"] -->|"simplified to"| F["Direct target_compatible_with"]
  B --> G["IE/Safari tests gated by OS"]
  D --> G
  F --> G

Loading

File Changes

1. java/test/org/openqa/selenium/devtools/NetworkInterceptorRestTest.java 🐞 Bug fix +0/-1

Remove selenium.skiptest assumption check

java/test/org/openqa/selenium/devtools/NetworkInterceptorRestTest.java

---

2. java/test/org/openqa/selenium/devtools/NetworkInterceptorTest.java 🐞 Bug fix +0/-1

Remove selenium.skiptest assumption check

java/test/org/openqa/selenium/devtools/NetworkInterceptorTest.java

---

3. java/test/org/openqa/selenium/javascript/ClosureTestSuite.java 🐞 Bug fix +0/-8

Remove skiptest BeforeAll method and imports

java/test/org/openqa/selenium/javascript/ClosureTestSuite.java

---

View more (7)

4. java/test/org/openqa/selenium/testing/JupiterTestBase.java 🐞 Bug fix +0/-7

Remove skiptest BeforeAll method and related imports

java/test/org/openqa/selenium/testing/JupiterTestBase.java

---

5. dotnet/private/dotnet_nunit_test_suite.bzl ⚙️ Configuration changes +3/-3

Remove skip-rbe tags from IE and Safari configs

dotnet/private/dotnet_nunit_test_suite.bzl

---

6. java/browsers.bzl ⚙️ Configuration changes +5/-15

Remove selenium.skiptest JVM flags from browser configs

java/browsers.bzl

---

7. java/private/selenium_test.bzl ✨ Enhancement +13/-12

Add target_compatible_with to browser configs and test rules

java/private/selenium_test.bzl

---

8. py/private/browsers.bzl ✨ Enhancement +8/-2

Add target_compatible_with and remove skip-rbe tags

py/private/browsers.bzl

---

9. rb/spec/tests.bzl ⚙️ Configuration changes +1/-5

Remove skip-rbe tags from IE and Safari browser configs

rb/spec/tests.bzl

---

10. py/BUILD.bazel ✨ Enhancement +2/-0

Add target_compatible_with to Python test rules

py/BUILD.bazel

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0)

1. target_compatible_with kwarg collision 🐞 Bug ≡ Correctness ⭐ New

Description

selenium_test now always passes target_compatible_with to junit5_test while still splatting
**stripped_args; if a caller provides target_compatible_with via **kwargs, Bazel will fail
with a duplicate keyword argument error. This makes `java_selenium_test_suite(...,
target_compatible_with=...)` (or any wrapper passing it through) break at analysis time.

Code

java/private/selenium_test.bzl[R116-117]

Evidence

The macro constructs stripped_args from kwargs and pops only data, jvm_flags, and tags
before calling junit5_test with both an explicit target_compatible_with and **stripped_args,
so a target_compatible_with key in kwargs would be forwarded and collide. The suite helper
forwards arbitrary **kwargs into selenium_test, making this a realistic call path.

java/private/selenium_test.bzl[84-141]
java/private/suite.bzl[97-127]

Agent prompt

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

## Issue description
`java/private/selenium_test.bzl` now sets `target_compatible_with = BROWSERS[browser]["target_compatible_with"]` in the `junit5_test(...)` call but does not remove `target_compatible_with` from `stripped_args` before forwarding `**stripped_args`. Any caller that passes `target_compatible_with` into `selenium_test` (directly or via `java_selenium_test_suite` which forwards `**kwargs`) will hit a duplicate keyword error during Bazel analysis.

## Issue Context
This macro already strips `data`, `jvm_flags`, and `tags` out of `kwargs` before splatting, but it does not strip `target_compatible_with`.

## Fix Focus Areas
- java/private/selenium_test.bzl[93-140]

## Suggested change
- Pop `target_compatible_with` from `stripped_args` and merge with the per-browser constraints, e.g.:
 - `extra_compat = stripped_args.pop("target_compatible_with", [])`
 - `target_compatible_with = BROWSERS[browser]["target_compatible_with"] + extra_compat`
- Apply the same pattern to the `*-remote` target generation as well.

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

---

2. Mixed macOS constraint labels 🐞 Bug ⚙ Maintainability

Description

.NET Safari tests are gated with @platforms//os:osx while Java/Python/Ruby and //common:macos
use @platforms//os:macos. This makes macOS compatibility gating inconsistent across bindings and
can cause Safari targets to be enabled/disabled differently depending on which macOS constraint a
chosen platform provides.

Code

dotnet/private/dotnet_nunit_test_suite.bzl[R92-93]

Evidence

The repo uses two different constraint values for “macOS” gating: .NET Safari uses
@platforms//os:osx, while the shared //common:macos config_setting and the Java/Python/Ruby
Safari browser configs all use @platforms//os:macos, demonstrating an inconsistency in how macOS
compatibility is expressed.

dotnet/private/dotnet_nunit_test_suite.bzl[71-105]
common/BUILD.bazel[29-42]
java/private/selenium_test.bzl[58-71]
py/private/browsers.bzl[96-108]
rb/spec/tests.bzl[130-159]

Agent prompt

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

### Issue description
`dotnet/private/dotnet_nunit_test_suite.bzl` gates Safari with `@platforms//os:osx`, but the rest of the repo (shared config settings + other language bindings) gates macOS with `@platforms//os:macos`. This inconsistency makes OS gating harder to reason about and risks cross-binding differences in which Safari targets are considered compatible.

### Issue Context
- `//common:macos` is defined using `@platforms//os:macos`, and Java/Python/Ruby Safari targets use the same value.
- Align the .NET Safari constraint value with the repo’s chosen macOS constraint.

### Fix Focus Areas
- dotnet/private/dotnet_nunit_test_suite.bzl[86-94]
- common/BUILD.bazel[34-42]
- java/private/selenium_test.bzl[65-71]
- py/private/browsers.bzl[102-107]
- rb/spec/tests.bzl[140-159]

### Expected change
- Update the .NET Safari entry to use `target_compatible_with = ["@platforms//os:macos"]` (or, if the repo decides `osx` is the canonical value, update the other bindings and `//common:macos` consistently).

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

---

Previous review results

Review updated until commit dfb8c18

Results up to commit 92a1eb5

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

1. Mixed macOS constraint labels 🐞 Bug ⚙ Maintainability

Description

.NET Safari tests are gated with @platforms//os:osx while Java/Python/Ruby and //common:macos
use @platforms//os:macos. This makes macOS compatibility gating inconsistent across bindings and
can cause Safari targets to be enabled/disabled differently depending on which macOS constraint a
chosen platform provides.

Code

dotnet/private/dotnet_nunit_test_suite.bzl[R92-93]

Evidence

The repo uses two different constraint values for “macOS” gating: .NET Safari uses
@platforms//os:osx, while the shared //common:macos config_setting and the Java/Python/Ruby
Safari browser configs all use @platforms//os:macos, demonstrating an inconsistency in how macOS
compatibility is expressed.

dotnet/private/dotnet_nunit_test_suite.bzl[71-105]
common/BUILD.bazel[29-42]
java/private/selenium_test.bzl[58-71]
py/private/browsers.bzl[96-108]
rb/spec/tests.bzl[130-159]

Agent prompt

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

### Issue description
`dotnet/private/dotnet_nunit_test_suite.bzl` gates Safari with `@platforms//os:osx`, but the rest of the repo (shared config settings + other language bindings) gates macOS with `@platforms//os:macos`. This inconsistency makes OS gating harder to reason about and risks cross-binding differences in which Safari targets are considered compatible.

### Issue Context
- `//common:macos` is defined using `@platforms//os:macos`, and Java/Python/Ruby Safari targets use the same value.
- Align the .NET Safari constraint value with the repo’s chosen macOS constraint.

### Fix Focus Areas
- dotnet/private/dotnet_nunit_test_suite.bzl[86-94]
- common/BUILD.bazel[34-42]
- java/private/selenium_test.bzl[65-71]
- py/private/browsers.bzl[102-107]
- rb/spec/tests.bzl[140-159]

### Expected change
- Update the .NET Safari entry to use `target_compatible_with = ["@platforms//os:macos"]` (or, if the repo decides `osx` is the canonical value, update the other bindings and `//common:macos` consistently).

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

---

[qodo-code-review]

Persistent review updated to latest commit dfb8c18