Skip to content

[py] generate -bidi test targets for bidi tests only#17630

Open
titusfortner wants to merge 1 commit into
trunkfrom
bidi_targets_py
Open

[py] generate -bidi test targets for bidi tests only#17630
titusfortner wants to merge 1 commit into
trunkfrom
bidi_targets_py

Conversation

@titusfortner
Copy link
Copy Markdown
Member

@titusfortner titusfortner commented Jun 4, 2026

🔗 Related Issues

Part of Python implementation of #17539

Currently, Python runs all classic tests with bidi enabled, even though none of those classic tests have bidi implementations.

⏱️ Measured per run

Before After Saved
Test targets (//py:all) 1443 1097 346
GHA chrome-bidi job (run time & wall-clock) ~51 min ~28 min ~23 min
RBE bidi-target runner time ~63 min ~21 min ~42 min

RBE saving is aggregate runner time only; wall-clock is unchanged (tests run in parallel).

💥 What does this PR do?

  • Generates -bidi targets from the BiDi test files only, not the whole classic suite re-run under --bidi.
  • Removes the -bidi-actions / -bidi-<feature> sub-suites (they held only classic tests) and collapses each browser to a single test-<browser>-bidi.
  • Adds an empty BIDI_IMPLEMENTATIONS hook for tests that should run under both modes.

🔧 Implementation Notes

  • BIDI_IMPLEMENTATIONS is the home for a feature with both a classic and a BiDi backend — files there stay classic and get a -bidi target to distinguish from BIDI_TESTS that require websocketUrl. Empty today.
  • The file list lives in the BUILD file because py_test_suite shards per file; pytest markers can't gate target generation.

🤖 AI assistance

  • AI assisted (complete below)
    • Tool(s): Claude Code
    • What was generated: the py/BUILD.bazel change and the target/runner-time analysis
    • I reviewed all AI output and can explain the change

🔄 Types of changes

  • Cleanup (formatting, renaming)

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jun 4, 2026

Review Summary by Qodo

Make release pipeline rerun-safe and generate BiDi targets for BiDi tests only

✨ Enhancement 🐞 Bug fix

Grey Divider

Walkthroughs

Description 
• Make release pipeline rerun-safe by handling already-published packages
• Generate BiDi test targets only from BiDi test files, not full classic suite
• Reduce test targets from 1443 to 1097 and save ~23 min in CI runtime
• Add error handling for npm and gem republish attempts, tag creation idempotency
Diagram 
flowchart LR
  A["Release Pipeline"] --> B["Rerun Safety"]
  B --> C["npm Publish Error Handling"]
  B --> D["Gem Publish Error Handling"]
  B --> E["Tag Creation Idempotency"]
  B --> F["PyPI Skip Existing"]
  A --> G["BiDi Test Generation"]
  G --> H["BiDi Tests Only"]
  G --> I["Remove Classic Reruns"]
  I --> J["Reduce Targets 1443→1097"]
  J --> K["Save ~23 min CI Time"]
Loading

Grey Divider

File Changes

1. rake_tasks/node.rake Error handling +9/-1

Add npm publish error handling for reruns

• Wrap Bazel.execute call in try-catch to handle npm republish conflicts
• Skip release if package already published with specific error message matching
• Allows rerunning release workflow without failing on existing versions

rake_tasks/node.rake

2. rake_tasks/ruby.rake Error handling +11/-9

Extract gem publishing with rerun safety

• Extract gem publishing logic into new publish_gem helper function
• Add error handling to catch and skip gem republish attempts
• Refactor nightly and standard release tasks to use new helper
• Reduces code duplication and enables rerun-safe gem publishing

rake_tasks/ruby.rake

3. .github/workflows/release.yml Error handling +23/-9

Make release workflow jobs rerun-safe and idempotent

• Rename create-language-tag job to create-tag and remove language filter condition
• Add idempotency check to skip tag creation if tag already exists
• Add Java release rerun safety check with error message for manual intervention
• Add skip-existing: true to PyPI publish action for idempotency
• Update job dependencies and Slack notification to use new job name

.github/workflows/release.yml

View more (1)
4. py/BUILD.bazel ✨ Enhancement +6/-69

Generate BiDi targets from BiDi tests only

• Add BIDI_IMPLEMENTATIONS empty list for tests with both classic and BiDi backends
• Replace three separate BiDi target generators (bidi-common, bidi-actions, bidi-features) with
 single test--bidi target
• Change BiDi test sources from full classic suite to only BIDI_TESTS + BIDI_IMPLEMENTATIONS
• Remove aggregation layer that combined bidi-common, bidi-actions, and bidi-features into single
 target
• Reduces test target count from 1443 to 1097 and CI runtime by ~23 minutes

py/BUILD.bazel

Grey Divider

Qodo Logo

@selenium-ci selenium-ci added C-py Python Bindings B-build Includes scripting, bazel and CI integrations labels Jun 4, 2026
@titusfortner titusfortner requested a review from cgoldberg June 4, 2026 18:55
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jun 4, 2026 •
edited
Loading

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (2)

Grey Divider


Remediation recommended

1. README bidi target outdated 🐞 Bug ⚙ Maintainability ⭐ New
Description 
README.md says //py:test-<browsername>-bidi runs the common suite (including BiDi tests), but
py/BUILD.bazel now generates test-<browser>-bidi from only BIDI_TESTS + BIDI_IMPLEMENTATIONS.
This misleads users about what test coverage the -bidi target provides.
Code

py/BUILD.bazel[R1019-1024]

+# Generate test-<browser>-bidi targets
[
    py_test_suite(
-        name = "test-%s-bidi-common" % browser,
+        name = "test-%s-bidi" % browser,
        size = "large",
-        srcs = glob(
-            [
-                "test/selenium/webdriver/common/**/*.py",
-                "test/selenium/webdriver/support/**/*.py",
-            ] + BROWSER_TESTS[browser]["browser_srcs"],
-            exclude = ACTIONS_TESTS + FEATURE_TESTS + DRIVER_FINDER_TESTS + ["test/selenium/webdriver/common/print_pdf_tests.py"] +
-                      BROWSER_TESTS[browser].get("extra_excludes", []),
-        ),
+        srcs = BIDI_TESTS + BIDI_IMPLEMENTATIONS,
Evidence 
The BUILD change makes test-<browser>-bidi include only files matched by BIDI_TESTS (plus
BIDI_IMPLEMENTATIONS), whereas README documents it as running common tests with BiDi included.

py/BUILD.bazel[1019-1039]
README.md[404-415]

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:test-<browsername>-bidi` no longer runs the classic/common test suite under `--bidi`; it now runs only BiDi-specific test files (`BIDI_TESTS`) plus any future dual-implementation tests (`BIDI_IMPLEMENTATIONS`). README.md still documents the old meaning, which can lead to confusion and incorrect assumptions about coverage.

### Issue Context
- `py/BUILD.bazel` changed `test-<browser>-bidi` to use `srcs = BIDI_TESTS + BIDI_IMPLEMENTATIONS`.
- `README.md` still claims `test-<browser>-bidi` runs “common tests … (include BiDi tests)”.

### Fix Focus Areas
- README.md[404-415]

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

2. BIDI_IMPLEMENTATIONS comment lacks rationale 📘 Rule violation ⚙ Maintainability
Description 
The new comment above BIDI_IMPLEMENTATIONS describes what the list contains, but not why it exists
or how it should be used/maintained. This reduces maintainability and increases the chance of
incorrect future target generation changes.
Code

py/BUILD.bazel[R781-782]

+# Tests that have bidi and classic implementations.
+BIDI_IMPLEMENTATIONS = []
Evidence 
Rule 8 requires comments to explain intent/rationale rather than restating obvious behavior. The
added comment # Tests that have bidi and classic implementations. does not capture why this list
is needed or when to use it, making future maintenance error-prone.

AGENTS.md: Write Comments That Explain Why (Not What)
py/BUILD.bazel[781-782]

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

## Issue description
A new comment describes what `BIDI_IMPLEMENTATIONS` is, but does not explain the rationale/intent (why this list exists and when a test should be added to it vs `BIDI_TESTS`).

## Issue Context
Per compliance guidance, comments should explain *why* (constraints/intent) rather than restating what the code does.

## Fix Focus Areas
- py/BUILD.bazel[781-782]

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

3. test-%s-bidi comment lacks rationale 📘 Rule violation ⚙ Maintainability
Description 
The updated comment for generating test-<browser>-bidi targets states what is generated, but not
the reason for the change (e.g., only BiDi-specific tests are included) or the intended invariant.
This makes it harder to safely modify the target generation later.
Code

py/BUILD.bazel[995]

+# Generate test-<browser>-bidi targets
Evidence 
Rule 8 requires comments to explain why rather than what. The comment `# Generate
test-<browser>-bidi targets` restates the code structure but does not explain the intent/invariant
behind the generation approach, reducing maintainability.

AGENTS.md: Write Comments That Explain Why (Not What)
py/BUILD.bazel[995-1000]

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 comment above the BiDi target generation explains what is being generated but not the rationale/intent behind the generation approach.

## Issue Context
Per compliance guidance, comments should document intent/invariants so future edits don't accidentally undo the underlying reason for the structure.

## Fix Focus Areas
- py/BUILD.bazel[995-1000]

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

View more (1)
4. Empty BiDi suite passes 🐞 Bug ☼ Reliability
Description 
test-<browser>-bidi now derives srcs solely from BIDI_TESTS + BIDI_IMPLEMENTATIONS, and the
py_test_suite macro will still emit a native.test_suite even when it contains zero tests. If the
glob stops matching (rename/move) or the list is accidentally emptied, CI can succeed while running
no BiDi tests (silent coverage loss).
Code

py/BUILD.bazel[1000]

+        srcs = BIDI_TESTS + BIDI_IMPLEMENTATIONS,
Evidence 
The BiDi suite’s srcs is now only BIDI_TESTS + BIDI_IMPLEMENTATIONS. The py_test_suite macro
always emits a native.test_suite from whatever tests it creates, and does not fail when it creates
none—so an empty BIDI_TESTS glob would yield an empty but “successful” suite.

py/BUILD.bazel[779-783]
py/BUILD.bazel[995-1015]
py/private/suite.bzl[18-57]

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 consolidated `test-<browser>-bidi` suite can silently contain zero tests if `BIDI_TESTS` stops matching files (or if `BIDI_IMPLEMENTATIONS` is incorrectly maintained), because `py_test_suite` will still create an empty `native.test_suite`.

## Issue Context
- `py_test_suite` only creates tests for entries in `srcs` that look like tests, and always emits a `native.test_suite` even if it found none.
- With this PR, BiDi suite `srcs` is limited to `BIDI_TESTS + BIDI_IMPLEMENTATIONS`, increasing the risk of an “empty suite that still passes”.

## Fix Focus Areas
- py/BUILD.bazel[779-783]
- py/BUILD.bazel[995-1015]

### Suggested fix
Add a BUILD-time assertion right after defining `BIDI_TESTS`/`BIDI_IMPLEMENTATIONS`:

```starlark
if len(BIDI_TESTS) == 0 and len(BIDI_IMPLEMENTATIONS) == 0:
   fail("No BiDi tests matched BIDI_TESTS/BIDI_IMPLEMENTATIONS; BiDi suite would be empty")
```

(Optionally, also ensure `BIDI_IMPLEMENTATIONS` does not overlap `BIDI_TESTS` to prevent duplicate-generated test rule names.)

ⓘ 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 be14921

Results up to commit 07bc7c9


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


Remediation recommended
1. Empty BiDi suite passes 🐞 Bug ☼ Reliability
Description 
test-<browser>-bidi now derives srcs solely from BIDI_TESTS + BIDI_IMPLEMENTATIONS, and the
py_test_suite macro will still emit a native.test_suite even when it contains zero tests. If the
glob stops matching (rename/move) or the list is accidentally emptied, CI can succeed while running
no BiDi tests (silent coverage loss).
Code

py/BUILD.bazel[1000]

+        srcs = BIDI_TESTS + BIDI_IMPLEMENTATIONS,
Evidence 
The BiDi suite’s srcs is now only BIDI_TESTS + BIDI_IMPLEMENTATIONS. The py_test_suite macro
always emits a native.test_suite from whatever tests it creates, and does not fail when it creates
none—so an empty BIDI_TESTS glob would yield an empty but “successful” suite.

py/BUILD.bazel[779-783]
py/BUILD.bazel[995-1015]
py/private/suite.bzl[18-57]

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 consolidated `test-<browser>-bidi` suite can silently contain zero tests if `BIDI_TESTS` stops matching files (or if `BIDI_IMPLEMENTATIONS` is incorrectly maintained), because `py_test_suite` will still create an empty `native.test_suite`.

## Issue Context
- `py_test_suite` only creates tests for entries in `srcs` that look like tests, and always emits a `native.test_suite` even if it found none.
- With this PR, BiDi suite `srcs` is limited to `BIDI_TESTS + BIDI_IMPLEMENTATIONS`, increasing the risk of an “empty suite that still passes”.

## Fix Focus Areas
- py/BUILD.bazel[779-783]
- py/BUILD.bazel[995-1015]

### Suggested fix
Add a BUILD-time assertion right after defining `BIDI_TESTS`/`BIDI_IMPLEMENTATIONS`:

```starlark
if len(BIDI_TESTS) == 0 and len(BIDI_IMPLEMENTATIONS) == 0:
   fail("No BiDi tests matched BIDI_TESTS/BIDI_IMPLEMENTATIONS; BiDi suite would be empty")
```

(Optionally, also ensure `BIDI_IMPLEMENTATIONS` does not overlap `BIDI_TESTS` to prevent duplicate-generated test rule names.)

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

Results up to commit ddcf49d


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


Remediation recommended
1. test-%s-bidi comment lacks rationale 📘 Rule violation ⚙ Maintainability
Description 
The updated comment for generating test-<browser>-bidi targets states what is generated, but not
the reason for the change (e.g., only BiDi-specific tests are included) or the intended invariant.
This makes it harder to safely modify the target generation later.
Code

py/BUILD.bazel[995]

+# Generate test-<browser>-bidi targets
Evidence 
Rule 8 requires comments to explain why rather than what. The comment `# Generate
test-<browser>-bidi targets` restates the code structure but does not explain the intent/invariant
behind the generation approach, reducing maintainability.

AGENTS.md: Write Comments That Explain Why (Not What)
py/BUILD.bazel[995-1000]

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 comment above the BiDi target generation explains what is being generated but not the rationale/intent behind the generation approach.

## Issue Context
Per compliance guidance, comments should document intent/invariants so future edits don't accidentally undo the underlying reason for the structure.

## Fix Focus Areas
- py/BUILD.bazel[995-1000]

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

2. BIDI_IMPLEMENTATIONS comment lacks rationale 📘 Rule violation ⚙ Maintainability
Description 
The new comment above BIDI_IMPLEMENTATIONS describes what the list contains, but not why it exists
or how it should be used/maintained. This reduces maintainability and increases the chance of
incorrect future target generation changes.
Code

py/BUILD.bazel[R781-782]

+# Tests that have bidi and classic implementations.
+BIDI_IMPLEMENTATIONS = []
Evidence 
Rule 8 requires comments to explain intent/rationale rather than restating obvious behavior. The
added comment # Tests that have bidi and classic implementations. does not capture why this list
is needed or when to use it, making future maintenance error-prone.

AGENTS.md: Write Comments That Explain Why (Not What)
py/BUILD.bazel[781-782]

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

## Issue description
A new comment describes what `BIDI_IMPLEMENTATIONS` is, but does not explain the rationale/intent (why this list exists and when a test should be added to it vs `BIDI_TESTS`).

## Issue Context
Per compliance guidance, comments should explain *why* (constraints/intent) rather than restating what the code does.

## Fix Focus Areas
- py/BUILD.bazel[781-782]

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

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jun 5, 2026 •
edited
Loading

Code review by qodo was updated up to the latest commit ddcf49d

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jun 6, 2026 •
edited
Loading

Code review by qodo was updated up to the latest commit be14921

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cgoldberg cgoldberg Awaiting requested review from cgoldberg

@AutomatedTester AutomatedTester Awaiting requested review from AutomatedTester

Assignees

No one assigned

Labels

B-build Includes scripting, bazel and CI integrations C-py Python Bindings

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@titusfortner @selenium-ci