Skip to content

[py] replace the blanket remote BiDi test suite with an opt-in list - #18023

Merged
titusfortner merged 3 commits into
SeleniumHQ:trunkfrom
titusfortner:py-remote-bidi-opt-in
Sep 11, 2026
Merged

[py] replace the blanket remote BiDi test suite with an opt-in list#18023
titusfortner merged 3 commits into
SeleniumHQ:trunkfrom
titusfortner:py-remote-bidi-opt-in

Conversation

@titusfortner

Copy link
Copy Markdown
Member

🔗 Related Issues

💥 What does this PR do?

  • Removes the remote BiDi test targets as unnecessary (everything unique about running BiDi over Grid is covered by Java tests)
  • Adds an empty REMOTE_BIDI_TESTS opt-in list, so a BiDi test file with Grid-specific behavior can run through Grid inside the existing Remote Tests job instead of a separate suite and job.

🔧 Implementation Notes

  • Saves about 30 minutes runner time per Python CI run
  • Saves about 45 EngFlow worker-minutes per uncached RBE run
  • The opt-in suite and its entry in test-<browser>-remote are only generated once REMOTE_BIDI_TESTS has entries, because Bazel treats an empty test_suite as every test in the package.

🤖 AI assistance

  • AI assisted (complete below)
    • Tool(s): Claude Code (Fable 5.1)
    • What was generated: the CI cost analysis, the removal, the opt-in list, this description
    • I reviewed all AI output and can explain the change

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

@selenium-ci selenium-ci added C-py Python Bindings B-build Includes scripting, bazel and CI integrations labels Sep 11, 2026
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Make remote Python BiDi tests opt-in within existing CI

✨ Enhancement ⚙️ Configuration changes 📝 Documentation 🕐 10-20 Minutes

Grey Divider

AI Description

• Removes the redundant standalone remote BiDi CI job and blanket test coverage.
• Adds an explicit opt-in list for Grid-specific BiDi test files.
• Includes opted-in BiDi targets within existing browser remote suites.
Diagram

graph TD
  A["Remote BiDi list"] --> B{"Tests selected?"} -->|Yes| C["BiDi targets"] --> D["Remote aggregate"] --> E["Remote CI job"] --> F["Selenium Grid"]
  B -->|No: skip BiDi| D
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Retain a filtered BiDi CI job
  • ➕ Preserves failure isolation for remote BiDi tests
  • ➕ Allows independent retries and CI ownership
  • ➖ Keeps a dedicated job with substantial runner overhead
  • ➖ Duplicates Grid coverage already provided elsewhere
  • ➖ Adds complexity even when no Grid-specific tests exist
2. Use test-level Bazel tags
  • ➕ Keeps Grid-specific intent close to each test
  • ➕ Allows query-based suite discovery without a central file list
  • ➖ Requires tagging conventions and broader macro changes
  • ➖ Can make test selection less explicit during review
  • ➖ Risks accidental inclusion through inconsistent tagging

Recommendation: The explicit opt-in list folded into existing remote aggregates is the best fit. It removes redundant CI cost, makes exceptional Grid coverage reviewable in one location, and correctly guards target generation to avoid Bazel's empty-test-suite behavior.

Files changed (3) +18 / -42

Documentation (1) +6 / -6
TESTING.mdDocument consolidated remote testing and BiDi opt-in +6/-6

Document consolidated remote testing and BiDi opt-in

• Removes commands for deleted remote BiDi targets and documents the consolidated remote suites. Explains how Grid-specific BiDi files can opt into remote execution through REMOTE_BIDI_TESTS.

py/TESTING.md

Other (2) +12 / -36
ci-python.ymlRemove the standalone remote BiDi CI job +0/-18

Remove the standalone remote BiDi CI job

• Deletes the dedicated workflow job that queried and ran the blanket remote BiDi suite. Opted-in coverage now runs through the existing Remote Tests job.

.github/workflows/ci-python.yml

BUILD.bazelConditionally add opted-in BiDi tests to remote suites +12/-18

Conditionally add opted-in BiDi tests to remote suites

• Introduces REMOTE_BIDI_TESTS and generates Chrome and Firefox remote BiDi targets only when the list has entries. Generated targets are folded into existing per-browser remote aggregates, while the standalone cross-browser remote BiDi suite is removed.

py/BUILD.bazel

@qodo-code-review

qodo-code-review Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📜 Skill insights (0)

Grey Divider


Action required

1. Two build comments narrate wiring 📘 Rule violation ⚙ Maintainability
Description
The comments above the remote test definitions describe which targets are generated, aggregated, and
conditionally included instead of explaining why that wiring is necessary. When the target
composition changes, maintainers must update both the expressions and their prose or leave an
inaccurate description behind.
Code

py/BUILD.bazel[R1143-1144]

+# Generate test-<browser>-remote-bidi targets (chrome and firefox only), folded
+# into test-<browser>-remote below.
Evidence
Compliance rule 8 requires comments to explain rationale rather than narrate code. The cited
comments repeat the target generation and conditional aggregation visible in the immediately
following expressions without documenting why that structure is needed.

AGENTS.md: Comments Must Explain Rationale Rather Than Restate Code
py/BUILD.bazel[1143-1144]
py/BUILD.bazel[1302-1303]

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

## Issue description
Two comments narrate target generation and aggregation already expressed directly by the surrounding build definitions, creating duplicate maintenance points.

## Fix Focus Areas
- py/BUILD.bazel[1143-1144]
- py/BUILD.bazel[1302-1303]

## Recommended Fix
Remove the comments that restate the generated target names, aggregation, and conditional inclusion. Retain or add comments only where they explain a non-obvious reason for the build structure.

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



Remediation recommended

2. One opt-in runs unrelated tests ✓ Resolved 🐞 Bug ≡ Correctness
Description
The remote suite's srcs combines REMOTE_BIDI_TESTS with every entry in BIDI_IMPLEMENTATIONS,
although only the former is documented as selecting files for Grid execution. Once the first remote
entry enables suite generation, all dual-mode files also execute remotely without opting in, and a
file placed in both lists is declared twice under the same target name.
Code

py/BUILD.bazel[1149]

+        srcs = REMOTE_BIDI_TESTS + BIDI_IMPLEMENTATIONS,
Evidence
BIDI_IMPLEMENTATIONS and REMOTE_BIDI_TESTS are separate classifications, while the documentation
says listing a file in the latter is what makes it run with --bidi --remote. The suite is enabled
based only on REMOTE_BIDI_TESTS, but then appends all BIDI_IMPLEMENTATIONS; moreover,
py_test_suite iterates the source list without deduplication and declares one target for every
occurrence, so an overlapping file causes a duplicate Bazel rule name.

py/BUILD.bazel[869-874]
py/BUILD.bazel[1143-1181]
py/private/suite.bzl[37-55]
py/TESTING.md[105-111]

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 remote BiDi suite appends `BIDI_IMPLEMENTATIONS` to the explicit opt-in list, allowing unselected tests into the suite and duplicate target declarations when a file appears in both lists.

## Fix Focus Areas
- py/BUILD.bazel[1143-1181]
- py/private/suite.bzl[37-55]
- py/TESTING.md[105-111]

## Recommended Fix
Set the remote BiDi suite's `srcs` to `REMOTE_BIDI_TESTS` only. Require dual-mode test files that need Grid-specific coverage to be explicitly added to that list like every other opted-in file.

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


Grey Divider

Context sources
Review mode: 🚀 Fast: This is a small, localized Bazel test-suite configuration change with contained CI behavior and no security, data, or public-contract risk.

Grey Divider

Tip of the day
💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread py/BUILD.bazel
Comment thread py/BUILD.bazel Outdated
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 2224af9

@titusfortner
titusfortner merged commit 2aa90e8 into SeleniumHQ:trunk Sep 11, 2026
33 checks passed
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants