Skip to content

Conversation

@titusfortner
Copy link
Member

@titusfortner titusfortner commented Jan 22, 2026

User description

Python is the only language that puts the config pieces before the test name. @cgoldberg / @navin772 any objection to just renaming them so it doesn't make my eyes twitch?

Before:

//py:test-firefox-remote-test/selenium/webdriver/support/event_firing_webdriver_tests.py

After:

//py:test/selenium/webdriver/support/event_firing_webdriver-firefox-remote

Eventually we move to more granular build files (colon further to the right), but that's low priority

//py/selenium/webdriver/support:event_firing_webdriver-firefox-remote

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Reorganizes Python test target naming convention

  • Moves config pieces after test name for consistency

  • Implements helper functions to strip test prefixes

  • Aligns Python naming with other language patterns


Diagram Walkthrough

flowchart LR
  A["Old naming:<br/>test-firefox-remote-test/path"] -->|"_suite_suffix()<br/>_strip_test_prefixes()"| B["New naming:<br/>path-test-firefox-remote"]
Loading

File Walkthrough

Relevant files
Enhancement
suite.bzl
Refactor test target naming with helper functions               

py/private/suite.bzl

  • Added _suite_suffix() function to extract config suffix from test
    suite name
  • Added _strip_test_prefixes() function to normalize file paths by
    removing test prefixes and extensions
  • Modified test name generation to use new helper functions, reversing
    the order of path and config components
  • Changed from name-src pattern to src-name pattern for consistency with
    other languages
+16/-1   

@selenium-ci selenium-ci added C-py Python Bindings B-build Includes scripting, bazel and CI integrations labels Jan 22, 2026
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jan 22, 2026

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
Copy link
Contributor

qodo-code-review bot commented Jan 22, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Support both test- and test_ prefixes

Update suite_suffix to also remove a test prefix, in addition to the existing
test- prefix, to handle more naming conventions.

py/private/suite.bzl [7-8]

 def _suite_suffix(name):
-    return name[len("test-"):] if name.startswith("test-") else name
+    if name.startswith("test-"):
+        return name[len("test-"):]
+    elif name.startswith("test_"):
+        return name[len("test_"):]
+    return name
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: This is a reasonable enhancement to make the _suite_suffix function more flexible by supporting both test- and test_ as prefixes, improving its usability.

Low
Switch to f-string formatting

Replace the existing %-style string formatting with an f-string for improved
readability when constructing the test_name.

py/private/suite.bzl [37]

-test_name = "%s-%s" % (_strip_test_prefixes(src), _suite_suffix(name))
+test_name = f"{_strip_test_prefixes(src)}-{_suite_suffix(name)}"
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly proposes using an f-string, which is the modern and more readable way to format strings in Python, improving code style.

Low
  • Update

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors Python Bazel test target naming to align with other language bindings in the Selenium monorepo. Previously, Python test targets had the configuration pieces (browser, mode) before the test name, which was inconsistent with other languages. This change reorders the naming to put the test path first, followed by the configuration suffix.

Changes:

  • Added helper function _strip_test_prefixes to normalize test file paths by removing common test-related prefixes and suffixes
  • Added helper function _suite_suffix to extract the configuration suffix from suite names
  • Modified test name generation to use format <test_path>-<config> instead of <config>-<test_path>

Copy link
Member

@navin772 navin772 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I was accustomed to the previous naming conventions 😅

Copy link
Member

@cgoldberg cgoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems more intuitive to have the full name of the test file like it was before.

Could it be something like:

//py:test/selenium/webdriver/support/event_firing_webdriver_tests.py-firefox-remote

I don't really care too much though... it's fine to merge this if you think it's better.

We should also update #16734 with an example of how to run a single file of tests.

@titusfortner
Copy link
Member Author

@cgoldberg yeah, I was going off ruby convention for it which removed "spec":

rb/spec/integration/selenium/webdriver/driver_spec.rb -> //rb/spec/integration/selenium/webdriver:driver-chrome-remote

But yeah, .NET & Java both keep "Test":

java/test/org/openqa/selenium/firefox/GeckoDriverServiceTest.java -> //java/test/org/openqa/selenium/firefox:GeckoDriverServiceTest
dotnet/test/common/RelativeLocatorTest.cs ->  //dotnet/test/common:RelativeLocatorTest

How about we just remove the .py:

//py:test/selenium/webdriver/support/event_firing_webdriver_tests-firefox-remote

@cgoldberg
Copy link
Member

How about we just remove the .py:

yea, I like that better.

@titusfortner titusfortner merged commit 61b9519 into trunk Jan 22, 2026
38 checks passed
@titusfortner titusfortner deleted the py_targets branch January 22, 2026 19:18
@cgoldberg
Copy link
Member

oops.. you just merged it... I'll change it in a new PR

titusfortner added a commit that referenced this pull request Jan 22, 2026
…6969)

* rearrange python test target naming

* [py] Keep test/ prefix to avoid module shadowing on Windows

* [py] Keep _tests suffix in test target names
titusfortner added a commit that referenced this pull request Jan 23, 2026
…6969)

* rearrange python test target naming

* [py] Keep test/ prefix to avoid module shadowing on Windows

* [py] Keep _tests suffix in test target names
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 Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants