Skip to content

Conversation

cgoldberg
Copy link
Member

@cgoldberg cgoldberg commented Oct 15, 2025

User description

💥 What does this PR do?

This PR updates the internal Python test suite so firefox tests use the main driver fixture instead of overriding it with their own driver defined in a different conftest.py.

This also updates ff_installs_addons_tests.py so it looks for test extensions in the local source tree if you are not running with bazel (only works if inside the py/ directory).

💡 Additional Considerations

This could be improved by locating extensions directory relative to __file__, so it would work from any directory.

🔄 Types of changes

  • Tests

PR Type

Tests


Description

  • Removed Firefox-specific driver fixture override in conftest.py

  • Updated Firefox addon tests to use no_driver_after_test marker

  • Added fallback logic for locating test extensions directory

  • Minor formatting cleanup in pyproject.toml


Diagram Walkthrough

flowchart LR
  A["Remove Firefox conftest.py"] --> B["Use shared driver fixture"]
  B --> C["Add no_driver_after_test markers"]
  C --> D["Update extensions path logic"]
Loading

File Walkthrough

Relevant files
Cleanup
conftest.py
Remove Firefox-specific driver fixture override                   

py/test/selenium/webdriver/firefox/conftest.py

  • Deleted entire file containing Firefox-specific driver fixture
  • Removes duplicate driver initialization logic
+0/-27   
Enhancement
ff_installs_addons_tests.py
Update addon tests with markers and path fallback               

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

  • Added @pytest.mark.no_driver_after_test to all test functions
  • Implemented fallback logic for locating extensions directory
  • Added support for running tests outside bazel environment
+14/-1   
Formatting
conftest.py
Minor formatting adjustment in conftest                                   

py/conftest.py

  • Added blank line before no_driver_after_test marker check
+1/-0     
pyproject.toml
Minor formatting cleanup in project config                             

py/pyproject.toml

  • Fixed formatting of closing brackets in lists
+2/-2     
Configuration changes
BUILD.bazel
Update build file to remove deleted conftest                         

py/BUILD.bazel

  • Removed reference to deleted firefox/conftest.py file
+0/-1     

@cgoldberg cgoldberg added C-py Python Bindings B-build Includes scripting, bazel and CI integrations labels Oct 15, 2025
@cgoldberg cgoldberg requested a review from navin772 October 15, 2025 16:14
@SeleniumHQ SeleniumHQ deleted a comment from qodo-merge-pro bot Oct 15, 2025
Copy link
Contributor

qodo-merge-pro bot commented Oct 15, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Improve test extension path discovery

Refactor the test extension path discovery in ff_installs_addons_tests.py to be
relative to the test file's location using file, instead of relying on
hardcoded relative paths that depend on the execution directory.

Examples:

py/test/selenium/webdriver/firefox/ff_installs_addons_tests.py [26-32]
for extensions_dir in (
    os.path.abspath("../../../../../../test/extensions/"),
    os.path.abspath("../common/extensions/"),
):
    extensions = extensions_dir
    if os.path.exists(extensions_dir):
        break

Solution Walkthrough:

Before:

# In ff_installs_addons_tests.py

# Path discovery relies on the current working directory
for extensions_dir in (
    os.path.abspath("../../../../../../test/extensions/"),
    os.path.abspath("../common/extensions/"),
):
    extensions = extensions_dir
    if os.path.exists(extensions_dir):
        break

# ... tests use `extensions` variable

After:

# In ff_installs_addons_tests.py
from pathlib import Path

# Path discovery is relative to the current file's location
this_file = Path(__file__).resolve()
# Example of a more robust path construction
extensions = this_file.parents[4] / "test" / "extensions"
if not extensions.exists():
    extensions = this_file.parent / "common" / "extensions"

# ... tests use `extensions` variable
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that using hardcoded relative paths in ff_installs_addons_tests.py is brittle and proposes a more robust solution using __file__, which is a significant improvement for test reliability.

Medium
Possible issue
Raise error if extensions directory is not found

Explicitly check if the extensions directory was found after searching possible
paths, and raise a descriptive FileNotFoundError if it was not.

py/test/selenium/webdriver/firefox/ff_installs_addons_tests.py [26-32]

-for extensions_dir in (
+possible_paths = [
     os.path.abspath("../../../../../../test/extensions/"),
     os.path.abspath("../common/extensions/"),
-):
-    extensions = extensions_dir
-    if os.path.exists(extensions_dir):
-        break
+]
+extensions = next((path for path in possible_paths if os.path.exists(path)), None)
+if extensions is None:
+    raise FileNotFoundError(f"Could not find extensions directory. Searched in {possible_paths}")
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that if the extensions directory is not found, the tests will fail with a less-than-clear error. Raising an explicit FileNotFoundError with the searched paths improves test failure diagnostics and makes debugging easier.

Low
  • Update

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! Good cleanup

@cgoldberg cgoldberg changed the title [py] Update Firefox tests to use driver fixture [py] Update internal Firefox tests to use driver fixture Oct 16, 2025
@cgoldberg cgoldberg merged commit a99c253 into SeleniumHQ:trunk Oct 16, 2025
22 checks passed
@cgoldberg cgoldberg deleted the py-firefox-driver-fixture branch October 16, 2025 14:53
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.

2 participants