[py][build]: consolidate Python CI tests by titusfortner · Pull Request #16766 · SeleniumHQ/selenium

titusfortner · 2025-12-20T16:58:21Z

User description

💥 What does this PR do?

The Ubuntu BiDi tests are already executing in RBE, so redundant here
If we need fsutil command, it should be for all windows bazel not just python
Can put the OS tests into the same matrix job

💡 Additional Considerations

Do we want to add firefox tests?
Do we want to test firefox-beta / chrome-beta? (maybe we should only be testing beta outside RBE to catch OS issues early — but that's a much bigger conversation)

🔄 Types of changes

Cleanup (formatting, renaming)

PR Type

Enhancement

Description

Consolidate separate browser test jobs into single matrix job
Move fsutil 8dot3 command to bazel.yml for all Windows tests
Eliminate redundant Ubuntu BiDi tests already running in RBE
Simplify CI workflow by combining OS-specific test matrices

Diagram Walkthrough

flowchart LR
  A["Separate browser-tests jobs<br/>ubuntu/windows/macos"] -->|consolidate| B["Single browser-tests job<br/>with OS matrix"]
  C["fsutil in ci-python.yml<br/>Windows only"] -->|move| D["fsutil in bazel.yml<br/>All Windows tests"]
  E["Redundant Ubuntu BiDi<br/>tests"] -->|remove| F["RBE handles BiDi<br/>testing"]

File Walkthrough

Relevant files

Configuration changes

bazel.yml `Add Windows 8dot3 short names configuration` .github/workflows/bazel.yml Add fsutil 8dot3 command execution for Windows environments Command disables 8dot3 short names to prevent file system issues Conditional execution only when os is windows	+3/-0
ci-python.yml `Consolidate browser test jobs into unified matrix` .github/workflows/ci-python.yml Consolidate three separate browser test jobs into single unified job Merge ubuntu, windows, and macos test matrices into one Remove duplicate fsutil command from Windows-specific job Eliminate redundant Ubuntu BiDi tests already running in RBE	+0/-39

qodo-code-review · 2025-12-20T16:58:41Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

qodo-code-review · 2025-12-20T16:59:27Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ ~~Restore missing BiDi tests~~ Suggestion Impact: The workflow was updated to include the BiDi test target by adding `//py:test-${{ matrix.browser }}-bidi` to the Bazel test command, restoring BiDi test coverage. The change does not match the suggested conditional logic and does not add the suffix to the `//py:common-...` target. code diff: - bazel test --local_test_jobs 1 --flaky_test_attempts 3 //py:common-${{ matrix.browser }} //py:test-${{ matrix.browser }} + bazel test --local_test_jobs 1 --flaky_test_attempts 3 //py:common-${{ matrix.browser }} //py:test-${{ matrix.browser }} //py:test-${{ matrix.browser }}-bidi Conditionally add the `-bidi` suffix to the `bazel test` command for non-Safari browsers to restore the BiDi tests that were removed during refactoring. .github/workflows/ci-python.yml [116-117] run: \| - bazel test --local_test_jobs 1 --flaky_test_attempts 3 --pin_browsers=true //py:common-${{ matrix.browser }} //py:test-${{ matrix.browser }} + BIDI="" + if [[ "${{ matrix.browser }}" != "safari" ]]; then + BIDI="-bidi" + fi + bazel test --local_test_jobs 1 --flaky_test_attempts 3 --pin_browsers=true //py:common-${{ matrix.browser }}$BIDI //py:test-${{ matrix.browser }} `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies that the refactoring inadvertently removed the BiDi tests for Windows, leading to a reduction in test coverage. Restoring these tests is critical for maintaining CI integrity.	High
General	Specify cmd shell for Windows step Add `shell: cmd` to the 'Disable 8dot3 short names' step to ensure the `fsutil` command runs reliably on Windows runners. .github/workflows/bazel.yml [165-167] - name: Disable 8dot3 short names if: inputs.os == 'windows' + shell: cmd run: fsutil 8dot3name set 0 Apply / Chat Suggestion importance[1-10]: 6 __ Why: This suggestion improves the robustness of the workflow by explicitly setting `shell: cmd` for a Windows-specific command, preventing potential failures if the default shell is not `cmd`.	Low
Update

cgoldberg

These changes remove all BiDi tests except on RBE. Shouldn't we also continue to test those on Windows?

Do we want to add firefox tests?

I tried adding Firefox on Windows on the GHA runners a while back, but ended up removing them. I can'tremember why, but I think it was because they were absurdly slow. I think if we can, we should add them.

titusfortner · 2025-12-20T20:06:15Z

All bidi tests being run by CI-Python also runs in RBE, so these are completely redundant.

I guess I'm thinking we may want to rework how we're doing bidi tests in general, since my plans for it were quite different than what we're doing now. But that's for a later PR with a longer conversation.

cgoldberg · 2025-12-20T21:29:12Z

All bidi tests being run by CI-Python also runs in RBE, so these are completely redundant.

They all run in a Linux container in RBE though. I think it's fine to remove Ubuntu from the ci-python.yml job matrix, but we should continue to run BiDi tests on the Windows runners there.

titusfortner · 2025-12-20T21:54:04Z

The CI job was only running bidi tests on ubuntu, I'm not removing anything that was previously running:

https://github.com/SeleniumHQ/selenium/pull/16766/changes#diff-a7ec86144013d78c9f1d734aa65a898e66b609638aa7b872491ee3858ebebbebL103-L110

titusfortner · 2025-12-20T21:55:37Z

Oh, I see it was also added below. got it.

navin772 · 2025-12-21T15:07:36Z

Lets change the PR title temporarily to test out the python CI, add [py].

navin772

Looks like we have some bazel target config issue. Python browser tests are failing due to this.

cgoldberg · 2025-12-21T15:41:59Z

@titusfortner the BiDi target needs to be changed from //py:test-${{ matrix.browser }}-bidi to //py:common-${{ matrix.browser }}-bidi.

Also, I'm not sure if we need to include the non-BiDi target (//py:common-${{ matrix.browser }}). We previously weren't running this. The non-BiDi tests will already get run, but with WebSockets enabled.

titusfortner · 2025-12-21T18:38:42Z

None of the other languages split common from "chrome only" in the test suites. Is there an issue with my combining them?

cgoldberg · 2025-12-21T19:07:52Z

Is there an issue with my combining them?

Do you mean removing thecommon-* targets from ./py/BUILD.bazel and adding the common tests to each test-* target? If so, I think that's fine... just remember to update README.md and the new TESTING.md because they reference both types of targets there.

navin772

LGTM!

titusfortner requested review from cgoldberg and navin772 December 20, 2025 16:58

selenium-ci added the B-build Includes scripting, bazel and CI integrations label Dec 20, 2025

qodo-code-review bot added the Review effort 2/5 label Dec 20, 2025

cgoldberg requested changes Dec 20, 2025

View reviewed changes

titusfortner added 2 commits December 20, 2025 16:39

consolidate python tests

1a40fd7

put bidi back

bf4f986

titusfortner force-pushed the py_tests branch from bd0d9c0 to bf4f986 Compare December 20, 2025 22:40

cgoldberg approved these changes Dec 21, 2025

View reviewed changes

navin772 changed the title ~~[build] consolidate Python CI tests~~ [py][build]: consolidate Python CI tests Dec 21, 2025

Merge branch 'trunk' into py_tests

194168f

navin772 reviewed Dec 21, 2025

View reviewed changes

all browser tests run in one suite

9a5fba7

titusfortner requested a review from cgoldberg December 21, 2025 22:04

titusfortner and others added 6 commits December 21, 2025 16:07

these fsutil shouldn't be necessary now [java] [dotnet]

8a7d224

Merge branch 'trunk' into py_tests

0372b20

fix failing tests

b5151bb

fix failing tests

5d6b7e6

remove marionette references

2f52ba8

let's debug this

6bd6d93

titusfortner added 2 commits December 22, 2025 07:32

let's not debug this as much?

fa25623

maybe the issue was single threaded required for windows

7d1a1cf

titusfortner requested a review from navin772 December 23, 2025 17:53

navin772 approved these changes Dec 23, 2025

View reviewed changes

Merge branch 'trunk' into py_tests

24de4e9

titusfortner merged commit 4d34904 into trunk Dec 24, 2025
32 checks passed

titusfortner deleted the py_tests branch December 24, 2025 17:27

Uh oh!

Conversation

titusfortner commented Dec 20, 2025 • edited by qodo-code-review bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

💥 What does this PR do?

💡 Additional Considerations

🔄 Types of changes

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-code-review bot commented Dec 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Compliance Guide 🔍

Uh oh!

qodo-code-review bot commented Dec 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Uh oh!

cgoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

titusfortner commented Dec 20, 2025

Uh oh!

cgoldberg commented Dec 20, 2025

Uh oh!

titusfortner commented Dec 20, 2025

Uh oh!

titusfortner commented Dec 20, 2025

Uh oh!

navin772 commented Dec 21, 2025

Uh oh!

navin772 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgoldberg commented Dec 21, 2025

Uh oh!

titusfortner commented Dec 21, 2025

Uh oh!

cgoldberg commented Dec 21, 2025

Uh oh!

navin772 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

titusfortner commented Dec 20, 2025 •

edited by qodo-code-review bot

Loading

qodo-code-review bot commented Dec 20, 2025 •

edited

Loading

qodo-code-review bot commented Dec 20, 2025 •

edited

Loading

navin772 left a comment •

edited

Loading