Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[py] add edge driver tests #14723

Merged
merged 15 commits into from
Nov 14, 2024
Merged

[py] add edge driver tests #14723

merged 15 commits into from
Nov 14, 2024

Conversation

Delta456
Copy link
Contributor

@Delta456 Delta456 commented Nov 7, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Add Python driver tests for Edge Browser.

Motivation and Context

These were missing and it is required to test if everything is working on Edge or not

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Tests


Description

  • Added new Python tests for the Edge browser driver to ensure functionality and reliability.
  • Created a pytest fixture for setting up and tearing down Edge driver instances.
  • Implemented tests for launching and closing Edge browser instances, including multiple instances.
  • Added tests to verify Edge WebDriver service logging and environment variable path usage.

Changes walkthrough 📝

Relevant files
Miscellaneous
__init__.py
Add initialization file for Edge driver tests                       

py/test/selenium/webdriver/edge/init.py

  • Added license and copyright information.
  • Created an empty __init__.py file for the Edge driver tests package.
  • +16/-0   
    Tests
    conftest.py
    Add pytest fixture for Edge driver setup                                 

    py/test/selenium/webdriver/edge/conftest.py

  • Added license and copyright information.
  • Defined a pytest fixture for setting up and tearing down an Edge
    driver instance.
  • +27/-0   
    edge_launcher_tests.py
    Implement Edge browser launch and multiple instance tests

    py/test/selenium/webdriver/edge/edge_launcher_tests.py

  • Added license and copyright information.
  • Implemented tests for launching and closing Edge browser instances.
  • Included tests for launching multiple Edge instances.
  • +34/-0   
    edge_service_tests.py
    Add tests for Edge WebDriver service and logging                 

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

  • Added license and copyright information.
  • Implemented tests for Edge WebDriver service logging.
  • Included tests for environment variable path usage.
  • +123/-0 

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    qodo-merge-pro bot commented Nov 7, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Naming Inconsistency
    The test function name 'test_we_can_launch_multiple_chrome_instances' mentions Chrome instead of Edge. This should be updated to reflect the correct browser being tested.

    Incorrect Log Message
    The test is checking for "Starting Microsoft Edge WebDriver" in the log output, but this might not be the correct message for Edge. Verify the actual log message produced by Edge WebDriver.

    Naming Inconsistency
    The class name 'TestChromeDriverService' should be updated to reflect Edge driver instead of Chrome driver.

    Copy link
    Contributor

    qodo-merge-pro bot commented Nov 7, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    ✅ Correct the test function name to accurately reflect the browser being tested
    Suggestion Impact:The test function name was changed from referring to Chrome instances to Edge instances, as suggested.

    code diff:

    -def test_we_can_launch_multiple_chrome_instances(clean_driver, clean_service):
    +def test_we_can_launch_multiple_edge_instances(clean_driver, clean_service):

    Rename the test function to reflect that it's testing Edge browser instances, not
    Chrome instances.

    py/test/selenium/webdriver/edge/edge_launcher_tests.py [28]

    -def test_we_can_launch_multiple_chrome_instances(clean_driver, clean_service):
    +def test_we_can_launch_multiple_edge_instances(clean_driver, clean_service):
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The function name incorrectly refers to Chrome instances instead of Edge instances. This change improves code clarity and accuracy, preventing potential confusion for developers.

    8
    ✅ Update the test function name to correctly identify the driver being tested
    Suggestion Impact:The function name was updated to reflect the correct driver being tested, aligning with the suggestion.

    code diff:

    -def test_uses_chromedriver_logging(clean_driver, driver_executable) -> None:
    +def test_uses_edgedriver_logging(clean_driver, driver_executable) -> None:

    Replace 'chromedriver' with 'msedgedriver' in the test function name to accurately
    reflect the driver being tested.

    py/test/selenium/webdriver/edge/edge_service_tests.py [29]

    -def test_uses_chromedriver_logging(clean_driver, driver_executable) -> None:
    +def test_uses_msedgedriver_logging(clean_driver, driver_executable) -> None:
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The function name incorrectly refers to ChromeDriver instead of EdgeDriver. This change enhances code accuracy and readability, aligning the test name with its actual purpose.

    8
    ✅ Correct the class name to accurately represent the service being tested
    Suggestion Impact:The class name was changed from TestChromeDriverService to TestEdgeDriverService, aligning with the suggestion to correct the class name to accurately represent the service being tested.

    code diff:

    -class TestChromeDriverService:
    +class TestEdgeDriverService:

    Rename the class to reflect that it's testing EdgeDriverService, not
    ChromeDriverService.

    py/test/selenium/webdriver/edge/edge_service_tests.py [106]

    -class TestChromeDriverService:
    +class TestEdgeDriverService:
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The class name incorrectly refers to ChromeDriverService instead of EdgeDriverService. This change improves code consistency and prevents potential misunderstandings about the tested service.

    8

    💡 Need additional feedback ? start a PR chat

    @VietND96
    Copy link
    Member

    VietND96 commented Nov 8, 2024

    Looks like this test is failing in RBE
    py/test/selenium/webdriver/edge/edge_launcher_tests.py::test_launch_and_close_browser FAILED [ 50%]

    @AutomatedTester
    Copy link
    Member

    tests are failing

    @Delta456
    Copy link
    Contributor Author

    Delta456 commented Nov 9, 2024

    It is passing locally for me

    image

    @shbenzer
    Copy link
    Contributor

    This is a good catch, should we make an issue for adding all test cases to edge?

    @AutomatedTester
    Copy link
    Member

    This isn't working. Please check the action

    Signed-off-by: Viet Nguyen Duc <[email protected]>
    @VietND96 VietND96 merged commit 0a724b1 into SeleniumHQ:trunk Nov 14, 2024
    1 check passed
    jkim2492 pushed a commit to jkim2492/selenium that referenced this pull request Nov 17, 2024
    ---------
    
    Signed-off-by: Viet Nguyen Duc <[email protected]>
    Co-authored-by: Viet Nguyen Duc <[email protected]>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants