-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
[py] Refactored conftest.py
in a more object oriented design approach
#15495
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
base: trunk
Are you sure you want to change the base?
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
conftest.py
in a more object oriented design approachconftest.py
in a more object oriented design approach
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.
@sandeepsuryaprasad thanks for the PR! Overall this looks really nice.
I pulled your branch and ran some tests, and noticed that the browser now closes and re-opens between every test. Previously, it was re-using the same browser instance between tests.
For example, if you run pytest py/test/selenium/webdriver/common/typing_tests.py
you will notice this. The same tests running against trunk behave different. This makes the tests take significantly longer to run. Can you look into that and make it so it behaves like it used to?
Also, the AI bot found some minor issues. Could you address these?:
#15495 (comment)
There are some |
@cgoldberg sure.. I figured out what went wrong.. I will fix it and push the code once agin. |
@sandeepsuryaprasad great, I'll give it another look once you update. |
@sandeepsuryaprasad I tried again after your latest changes and there are still issues.
If I try to run tests with the argument Similarly, if I run tests with the argument
I haven't looked into what is causing these, but they need to be fixed before we can merge this. |
@cgoldberg I am working on it..running tests on my local machine.. |
@cgoldberg can you try it now.. except remote tests all other tests are passing when I run CI on my machine. Regarding closing and re-opening of the browser for every test, the scope of the fixture |
@sandeepsuryaprasad The issues I mentioned in my last comment are still there... I don't see any changes that fix them. As far as the browser closing/re-opening... I haven't investigated the cause, but it didn't do that before (run the tests against trunk to see). We don't want to introduce that change, so it needs to function the same way as it did previously. |
@cgoldberg I have fixed
|
@cgoldberg working on browser closing/re-opening issue. |
@cgoldberg Looks like there is some bug in the below code that I have taken from But the variable
If you carefully observe the execution, except for the above tests, for all other tests a new instance of chrome opens (setup part) and closes (teardown part,
But in the PR that I have raised, the teardown part
Please let me know your thoughts on this. Thanks! |
2c5d02f
to
1e518ac
Compare
@cgoldberg I have fixed all the review comments. After the code change in this PR, the behaviour is exactly same as it was on trunk. Can you please re-trigger the CI. I have executed all tests on my local and it's passing.
|
I just triggered CI again. |
14fc4f6
to
3caaf8c
Compare
@cgoldberg there were some logical issues with newly developed
Could you please validate once and re-trigger the CI. Hopefully this time everything should be okay. It was little bit trickier than I thought it would be! |
I just triggered CI. |
@cgoldberg looks like the tests are passing except some flakey ones |
executed //py:common-chrome-bidi-test/selenium/webdriver/common/api_example_tests.py separately once again..
|
@sandeepsuryaprasad However, I found one issue with the tests for for example, if I try to run any test using the Note the Capitalization... It is trying to instantiate This is caused by the following line inside your We can't make the assumption that the class name is always the capitalized You could fix it by replacing that line with:
... but perhaps there is a nicer solution? (BTW, I can't get the |
@cgoldberg I am working on this issue to provide an elegant solution.. |
@cgoldberg I have modified the code, introduced one more level of abstraction for handling driver classes and options classes. Please let me know your feedback. I ran all the tests on my local and it's passing. |
No, it isn’t. Please run tests locally using: bazel test //py:<bazel_tests> |
@shbenzer do you want me to run all browser tests as well as remote tests on my local machine. |
Considering you are editing conftest.py, that’s what I would do. |
@cgoldberg @shbenzer @titusfortner I ran all the tests on my local machine... below are the results Unit Tests
Chrome Tests
Firefox Tests
Edge Tests
Remote Tests
Two remote tests fails with following exception
|
@cgoldberg @shbenzer @titusfortner I ran all Bazel tests both on trunk as well as on my branch and the results are same. The flakey |
All of the Python tests on GitHub are failing for:
The capabilities being sent to start the driver in this code are missing the location to the browser, which on RBE is the pinned browser. Take a look at how the code in trunk is passing the value and make sure you haven't changed the logic with this code. |
c810e48
to
b85d14b
Compare
b85d14b
to
2f66da6
Compare
@cgoldberg , @shbenzer, @titusfortner thanks for helping me in finding out the issue. While raising this PR, I realised that I had completely missed the below part of the code, which was there on trunk but not on my branch. I have fixed it in my latest commit.
Also, I have made sure that the logic on trunk and logic on my branch is exactly same apart from structural changes. I have run all bazel tests and CI from my local (it passes). Can you please re-trigger the CI (especially RBE tests). |
It's running now |
Thanks!! Appreciate your quick response and support. |
@cgoldberg , @shbenzer , @titusfortner finally all RBE tests are passing except one JS test.
|
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.
Refactored
conftest.py
in a more object oriented design approachMotivation and Context
Refactored
conftest.py
in a more object oriented design approach so that the code is more readable and easy to make changes in the future.Types of changes
Checklist
PR Type
Enhancement
Description
Refactored
conftest.py
to an object-oriented design for better readability.Introduced a
Driver
class to encapsulate driver-related logic and properties.Simplified driver initialization and teardown using the
Driver
class.Removed redundant functions and streamlined driver configuration handling.
Changes walkthrough 📝
conftest.py
Refactored `conftest.py` with a `Driver` class
py/conftest.py
Driver
class to encapsulate driver logic.design.