Skip to content

[py] Fix failing test for Edge logging#15605

Merged
cgoldberg merged 1 commit intoSeleniumHQ:trunkfrom
cgoldberg:py-fix-edge-service-logging-test
Apr 10, 2025
Merged

[py] Fix failing test for Edge logging#15605
cgoldberg merged 1 commit intoSeleniumHQ:trunkfrom
cgoldberg:py-fix-edge-service-logging-test

Conversation

@cgoldberg
Copy link
Member

@cgoldberg cgoldberg commented Apr 10, 2025 •
edited by qodo-code-review bot
Loading

User description

💥 What does this PR do?

This is a fix to the internal Python test suite.

This PR fixes the test_uses_edgedriver_logging test in py/test/selenium/webdriver/edge/edge_service_tests.py.

Previously, this test was failing because it was attempting to launch 2 browsers using the same Service object. This is not possible and results in an error:

E       selenium.common.exceptions.SessionNotCreatedException: Message: session not created
E       from unknown error: cannot find msedge binary

This PR creates 2 Service objects so each driver instance can use its own.

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Bug fix

Description

  • Fixed failing test_uses_edgedriver_logging in Edge service tests.

  • Replaced single Service object with two separate instances.

  • Ensured proper cleanup of driver instances and log file.

Changes walkthrough 📝

Relevant files
Bug fix
edge_service_tests.py
Fix Edge logging test with separate Service instances       

py/test/selenium/webdriver/edge/edge_service_tests.py

  • Replaced single Service object with two separate instances.
  • Updated test logic to use distinct services for each driver.
  • Added conditional cleanup for driver1 to avoid potential errors.
    		• +13/-4   
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @selenium-ci selenium-ci added the C-py Python Bindings label Apr 10, 2025
    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Apr 10, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ No major issues detected

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Apr 10, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Use unique log files

    Using the same log file for both service instances can lead to race conditions
    and unpredictable test results. Create unique log files for each service to
    ensure proper isolation.

    py/test/selenium/webdriver/edge/edge_service_tests.py [33-43]

     service1 = Service(
-    log_output=log_file,
+    log_output="msedgedriver1.log",
     service_args=service_args,
     executable_path=driver_executable,
 )
 
 service2 = Service(
-    log_output=log_file,
+    log_output="msedgedriver2.log",
     service_args=service_args,
     executable_path=driver_executable,
 )
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: Using separate log files for each service would prevent potential race conditions and make the test more reliable. This is a valid concern as concurrent writes to the same log file could lead to unpredictable test behavior.

    		Medium
    Check file exists before removal

    The test will fail if the log file doesn't exist when attempting to remove it.
    Add a check to verify the file exists before trying to delete it.

    py/test/selenium/webdriver/edge/edge_service_tests.py [54-59]

     finally:
     if driver1:
         driver1.quit()
     if driver2:
         driver2.quit()
-    os.remove(log_file)
+    if os.path.exists(log_file):
+        os.remove(log_file)
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    __

    Why: Adding a check for file existence before removal prevents potential FileNotFoundError exceptions if the log file wasn't created for some reason. This improves error handling and makes the test more robust.

    		Low
    • More

    @cgoldberg cgoldberg merged commit e8fae63 into SeleniumHQ:trunk Apr 10, 2025
    20 checks passed
    @cgoldberg cgoldberg deleted the py-fix-edge-service-logging-test branch April 10, 2025 04:26
    @cgoldberg cgoldberg mentioned this pull request Apr 18, 2025
    Merged
    @garrettgregor garrettgregor mentioned this pull request May 3, 2025
    Merged
    This was referenced Sep 12, 2025
    Closed
    Closed
    Merged
    This was referenced Sep 19, 2025
    Closed
    Closed
    Closed
    This was referenced Oct 1, 2025
    Closed
    Closed
    Closed
    @dependabot dependabot bot mentioned this pull request Oct 24, 2025
    Closed
    This was referenced Dec 21, 2025
    Open
    Closed
    Closed
    Closed
    This was referenced Jan 19, 2026
    Open
    Open
    Open
    Merged
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    C-py Python Bindings Review effort 1/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @cgoldberg @selenium-ci