Skip to content

Conversation

@cgoldberg
Copy link
Member

@cgoldberg cgoldberg commented Jan 30, 2026

💥 What does this PR do?

This PR allows extensions tests to work when not running bazel.

Currently, there are 2 test files that use bazel Runfiles to locate web extensions when running tests. If you try to run these directly with pytest (without bazel), they will fail because Runfiles doesn't exist.

This PR adds a function to locate extensions in the local source tree when Runfiles isn't installed, and updates tests to use it for locating extensions.

This also fixes a subtle bug where the new ruff-check.py was giving incorrect results when finding import ordering issues.

It also skips devtools when running ruff-check.py and ruff-format.py from bazel.

🔄 Types of changes

  • Dev/Test
  • Bug fix (backwards compatible)

@qodo-code-review
Copy link
Contributor

PR Type

Bug fix, Tests


Description

  • Add get_extensions_location() function to locate extensions with or without bazel

  • Remove direct Runfiles dependency from test files

  • Update extension tests to use new location function

  • Enable tests to run with pytest without bazel


File Walkthrough

Relevant files
Enhancement
conftest.py
Add extension location helper function                                     

py/conftest.py

  • Added get_extensions_location() function that uses Runfiles when
    available, falls back to local source tree
  • Function checks if Runfiles is available and uses appropriate path
    resolution method
  • Provides centralized extension location logic for test files
+13/-0   
Bug fix
bidi_webextension_tests.py
Refactor to use centralized extension location                     

py/test/selenium/webdriver/common/bidi_webextension_tests.py

  • Removed direct python.runfiles import dependency
  • Replaced inline Runfiles initialization with call to
    get_extensions_location()
  • Changed variable name from lowercase extensions to uppercase
    EXTENSIONS constant
  • Updated all test methods to use new EXTENSIONS constant
+10/-13 
ff_installs_addons_tests.py
Refactor to use centralized extension location                     

py/test/selenium/webdriver/firefox/ff_installs_addons_tests.py

  • Removed direct python.runfiles import dependency
  • Replaced inline Runfiles initialization with call to
    get_extensions_location()
  • Changed variable name from lowercase extensions to uppercase
    EXTENSIONS constant
  • Updated all test functions to use new EXTENSIONS constant
+10/-10 
Formatting
firefox_profile_tests.py
Minor import formatting cleanup                                                   

py/test/selenium/webdriver/firefox/firefox_profile_tests.py

  • Minor formatting change removing blank line after import statement
+0/-1     

@cgoldberg cgoldberg self-assigned this Jan 30, 2026
@selenium-ci selenium-ci added the C-py Python Bindings label Jan 30, 2026
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jan 30, 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: 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

🔴
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Missing fallback checks: get_extensions_location() does not handle Runfiles.Create() returning None or Rlocation()
returning an invalid path, which can lead to unclear runtime failures when the extensions
directory cannot be resolved.

Referred Code
if Runfiles is not None:
    r = Runfiles.Create()
    extensions = r.Rlocation("selenium/py/test/extensions")
else:
    extensions = str((Path(__file__).parent.parent / "common" / "extensions").resolve())
return extensions

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 30, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix fallback extensions path

In get_extensions_location, correct the fallback path for extensions to point to
py/test/extensions instead of common/extensions to ensure tests can find the
necessary files.

py/conftest.py [191-192]

 else:
-    extensions = str((Path(__file__).parent.parent / "common" / "extensions").resolve())
+    extensions = str((Path(__file__).parent / "test" / "extensions").resolve())
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion corrects a faulty file path in the new get_extensions_location function, fixing a bug that would prevent tests from finding extensions when not run with Bazel.

High
Use temporary directory for unzipping

In test_install_uninstall_signed_addon_dir, use the pytest tmp_path fixture to
create a temporary directory for extracting the addon, which avoids polluting
the source tree and prevents potential permission errors.

py/test/selenium/webdriver/firefox/ff_installs_addons_tests.py [87-106]

 @pytest.mark.no_driver_after_test
-def test_install_uninstall_signed_addon_dir(driver, pages):
-    zip = os.path.join(EXTENSIONS, "webextensions-selenium-example.zip")
-
-    target = os.path.join(EXTENSIONS, "webextensions-selenium-example-unzip")
-    with zipfile.ZipFile(zip, "r") as zip_ref:
+def test_install_uninstall_signed_addon_dir(driver, pages, tmp_path):
+    zip_path = os.path.join(EXTENSIONS, "webextensions-selenium-example.zip")
+    target = tmp_path / "webextensions-selenium-example-unzip"
+    with zipfile.ZipFile(zip_path, "r") as zip_ref:
         zip_ref.extractall(target)
 
-    id = driver.install_addon(target)
+    id = driver.install_addon(str(target))
     assert id == "webextensions-selenium-example-v3@example.com"
 
     pages.load("blank.html")
     injected = WebDriverWait(driver, timeout=2).until(
         lambda dr: dr.find_element(By.ID, "webextensions-selenium-example")
     )
     assert injected.text == "Content injected by webextensions-selenium-example"
 
     driver.uninstall_addon(id)
     driver.refresh()
     assert len(driver.find_elements(By.ID, "webextensions-selenium-example")) == 0
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that extracting test artifacts into the source directory is poor practice and can cause permission errors, proposing the standard tmp_path fixture for improved test robustness.

Medium
General
Add existence check for extensions dir

In get_extensions_location, add a check to verify that the extensions directory
exists and raise a FileNotFoundError if it does not, to ensure a clear failure
message.

py/conftest.py [193]

+if not Path(extensions).exists():
+    raise FileNotFoundError(f"Extensions directory not found: {extensions}")
 return extensions
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion improves error handling by adding a check for the existence of the extensions directory, which provides clearer and faster feedback if the path is incorrect.

Low
Avoid shadowing built-in zip

In test_install_uninstall_signed_addon_dir, rename the zip variable to zip_path
to avoid shadowing the built-in zip function.

py/test/selenium/webdriver/firefox/ff_installs_addons_tests.py [89]

-zip = os.path.join(EXTENSIONS, "webextensions-selenium-example.zip")
+zip_path = os.path.join(EXTENSIONS, "webextensions-selenium-example.zip")
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly points out that the variable zip shadows a Python built-in, and renaming it to zip_path improves code clarity and prevents potential issues.

Low
Learned
best practice
Avoid import-time path resolution

Avoid resolving the extensions directory at module import time; call
get_extensions_location() inside the tests (or via a session-scoped fixture) to
keep module import side-effect-free and make failures easier to diagnose.

py/test/selenium/webdriver/common/bidi_webextension_tests.py [30]

-EXTENSIONS = get_extensions_location()
+def _extensions_dir():
+    return get_extensions_location()
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Avoid brittle import-time filesystem resolution; resolve paths lazily (or via fixtures) so failures are clearer and execution remains predictable across environments.

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 enables Python extension tests to run outside of the Bazel build system by adding a fallback mechanism for locating test extensions. It also fixes import ordering detection issues with ruff and excludes auto-generated devtools files from linting.

Changes:

  • Added get_extensions_location() function to locate extensions via Runfiles when using Bazel or via local source tree otherwise
  • Fixed import ordering to place conftest imports after first-party imports (selenium, test)
  • Configured ruff isort with known-local-folder = ["conftest"] to properly detect import ordering issues
  • Excluded common/devtools/ directory from ruff linting

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
py/conftest.py Added get_extensions_location() helper function that uses Runfiles when available or falls back to local path
py/test/selenium/webdriver/firefox/firefox_profile_tests.py Reordered imports to place conftest import after selenium imports
py/test/selenium/webdriver/firefox/ff_installs_addons_tests.py Replaced direct Runfiles usage with get_extensions_location() function
py/test/selenium/webdriver/common/bidi_webextension_tests.py Replaced direct Runfiles usage with get_extensions_location() function
py/pyproject.toml Added ruff isort configuration for known-local-folder to fix import ordering detection
py/private/ruff_check.py Added devtools exclusion to prevent linting third-party protocol files

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!
Nice work

@cgoldberg cgoldberg merged commit 3ef3d52 into SeleniumHQ:trunk Feb 1, 2026
29 checks passed
@cgoldberg cgoldberg deleted the py-extensions-runfiles branch February 1, 2026 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants