Skip to content

Conversation

titusfortner
Copy link
Member

@titusfortner titusfortner commented Jan 2, 2024

Description

  • Selenium Manager does not know about Options/Capabilities
  • DriverFinder is not static and returns browser & driver paths in separate methods
  • Browser classes change options only if applicable
  • More reasonable method names

Motivation and Context

Comments

  • Am I doing memoization correctly?
  • Am I using private methods correctly?

@codecov-commenter
Copy link

codecov-commenter commented Jan 2, 2024

Codecov Report

Attention: 49 lines in your changes are missing coverage. Please review.

Comparison is base (097d301) 58.07% compared to head (7058453) 57.97%.
Report is 3 commits behind head on trunk.

❗ Current head 7058453 differs from pull request most recent head 9b6c810. Consider uploading reports for the commit 9b6c810 to get more accurate results

Files Patch % Lines
py/selenium/webdriver/common/driver_finder.py 34.88% 22 Missing and 6 partials ⚠️
py/selenium/webdriver/common/selenium_manager.py 54.05% 12 Missing and 5 partials ⚠️
py/selenium/webdriver/chromium/webdriver.py 60.00% 1 Missing and 1 partial ⚠️
py/selenium/webdriver/webkitgtk/webdriver.py 0.00% 1 Missing ⚠️
py/selenium/webdriver/wpewebkit/webdriver.py 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##            trunk   #13387      +/-   ##
==========================================
- Coverage   58.07%   57.97%   -0.10%     
==========================================
  Files          88       88              
  Lines        5340     5366      +26     
  Branches      224      231       +7     
==========================================
+ Hits         3101     3111      +10     
- Misses       2015     2024       +9     
- Partials      224      231       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@AutomatedTester AutomatedTester left a comment

Choose a reason for hiding this comment

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

I think this looks good enough. I am not sure we need to have the Rakefile stuff as that should all be handled by bazel already

@titusfortner
Copy link
Member Author

Thanks, I would have merged this for 4.17 but I wanted to get .NET happy first as well before merging everything else. 😦

The Rakefile stuff includes tasks to make it so I can test and debug locally with PyCharm, but also each binding has its own syntax for testing, which is a pain when I'm trying to remember which are which.

Python is a good example of this.
You think you want:

bazel test //py:test-chrome

but actually it is:

bazel test //py:common-chrome

So I create task that does both and outputs results

My plan is to support a wrapper for each language with defaults like:

./go <language>:test:unit
./go <language>:test:<browser>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-py Python Bindings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants