You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
• 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
➖ 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.
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.
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.
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.
+# 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.
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
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.
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.
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
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.
Tip of the day
💡 Did you know, you can switch off images and animations for a plain-text comment
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
B-buildIncludes scripting, bazel and CI integrationsC-pyPython Bindings
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Related Issues
💥 What does this PR do?
REMOTE_BIDI_TESTSopt-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
test-<browser>-remoteare only generated onceREMOTE_BIDI_TESTShas entries, because Bazel treats an emptytest_suiteas every test in the package.🤖 AI assistance
💡 Additional Considerations
🔄 Types of changes