-
Notifications
You must be signed in to change notification settings - Fork 3
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
Fix selenium webdriver on WSL #99
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #99 +/- ##
==========================================
+ Coverage 83.26% 83.30% +0.03%
==========================================
Files 42 42
Lines 5636 5636
Branches 768 768
==========================================
+ Hits 4693 4695 +2
+ Misses 741 740 -1
+ Partials 202 201 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I only have two suggestions for improving the docstring and maybe renaming the environment variable.
test/test_web.py
Outdated
|
||
@staticmethod | ||
def share_selenium_web_driver() -> bool: | ||
"""Should the selenium driver be shared or not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rather: "Determine whether the selenium driver shall be shared between all consecutive tests in this class"
test/test_web.py
Outdated
Checks whether the environment variable SHARE_SELENIUM_WEB_DRIVER is set. | ||
On WSL default ist not sharing the driver since this causes concurrency conflicts. | ||
""" | ||
if (value := os.getenv('SHARE_SELENIUM_WEB_DRIVER')) is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to prepend "SHC_TEST_" to the environment variable name. We already have the env variable SHC_TEST_MSQL_URL
for controlling the MySQL interface tests and I think it would be good to have them all in a common "namespace".
I'd actually rename the variable to SHC_TEST_REUSE_WEB_DRIVER
, but I don't have strong opinions about the precise name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally fine w/ your change requests. Done.
Sharing the selenium driver in the web tests is now optional. Since sharing the web driver causes concurrency issues on e.g. WSL it can now be configured by setting the environment variable
SHARE_SELENIUM_WEB_DRIVER
.Running on WSL defaults to not share the driver but can be overwritten w/ above environment variable.
All other OS will share the driver by default unless
SHARE_SELENIUM_WEB_DRIVER
is set to True.