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

[wip][py] Expose browser specific functionalities for remote webdrivers #11500

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

j3soon
Copy link
Contributor

@j3soon j3soon commented Dec 31, 2022

Description

  • Expose browser specific functionalities for remote webdrivers in Python
  • Add corresponding tests

Motivation and Context

Implements #11483.

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.

@codecov-commenter
Copy link

codecov-commenter commented Dec 31, 2022

Codecov Report

Attention: Patch coverage is 50.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 54.51%. Comparing base (802c1f2) to head (aff181f).
Report is 3075 commits behind head on trunk.

Files with missing lines Patch % Lines
py/selenium/webdriver/chromium/webdriver.py 0.00% 3 Missing ⚠️
py/selenium/webdriver/firefox/webdriver.py 40.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk   #11500      +/-   ##
==========================================
- Coverage   54.54%   54.51%   -0.03%     
==========================================
  Files          85       85              
  Lines        5627     5631       +4     
  Branches      243      243              
==========================================
+ Hits         3069     3070       +1     
- Misses       2315     2318       +3     
  Partials      243      243              

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


🚨 Try these New Features:

@j3soon j3soon changed the title Expose browser specific functionalities for remote webdrivers in Python [wip][py] Expose browser specific functionalities for remote webdrivers Dec 31, 2022
@titusfortner
Copy link
Member

@j3soon are you still working on this? What is the status?

@j3soon
Copy link
Contributor Author

j3soon commented Apr 18, 2023

@titusfortner, thanks for asking!

I have finished implementing the functionalities in this PR (commit aff181f). The corresponding user API can be found in the Usage example section in #11483, and have been tested manually (by myself) a while ago.

However, I didn't have time to convert those examples to automated Selenium test cases, which is required in the contributing guideline.

I would appreciate if someone could help convert those examples into Selenium test cases. Otherwise, I plan to write the test cases myself in the far future (maybe in 2024 or 2025).

@titusfortner
Copy link
Member

hah, yes, everyone is so busy at this point. Thanks for the update. We'll see what we can do for it. I'd definitely like to see this functionality added.

@isaulv
Copy link
Contributor

isaulv commented Aug 15, 2023

I don't like this code, and would not want to merge it in. But the current situation does bother me a lot, and there's is one hack that tries to load an appropriate browser specific remoteConnection. But your PR seems to show that more work needs to be done (which is something I was always aware of, when I first encountered a subclassed WebElement in the form of FirefoxWebelement).

@isaulv isaulv added the C-py label Aug 15, 2023
@j3soon
Copy link
Contributor Author

j3soon commented Aug 15, 2023

Hi @isaulv , I acknowledge that this patch isn't ready to be merged yet, as indicated by the [wip] prefix in the title.

At the moment, the patch only serves as a proof-of-concept, showing that these features could be easily implemented using python-specific hacks, as discussed in issue #11483.

If you've managed to come up with a better implementation, please feel free to replace the current one.

It's worth noting that not merging this patch will not prevent the practical use of these functionalities. Users have the option to apply the python-specific hacks on their end, based on the remote driver documentation. This is one of the reasons why I haven't prioritized continuing to work on this patch.

@titusfortner
Copy link
Member

So, the underlying feature here, is that Java, Ruby & .NET all have ways to implement browser specific functionality in Remote Driver.

https://docs.saucelabs.com/web-apps/automated-testing/selenium/selenium4/#network-conditions

We need to figure out some way, some how to provide this functionality in Python, and I have no idea the right way to do that. I know David doesn't like the idea of mixins, which is what Ruby is doing. Java is doing pure magic, and .NET is kind of magic that you have to be explicit about. 😂

@isaulv
Copy link
Contributor

isaulv commented Aug 15, 2023

@j3soon I want to thank you for pointing out possible solutions. It's a big help for other users who need to handle this issue. I didn't mean to sound like a negative downer, I just want to have a solution that makes sense for a majority of Python programmers.

@titusfortner I agree with David, that mixins are a bad idea. I would like an automatic solution since Python is dynamic enough for this to happen. I don't like mixins for many reasons, but the main argument is that multiple inheritance can add a lot of complexity to debugging.
My goal is to have a solution that works with simple OO constructs but doesn't require monkeypatching or weird code. To that end, I'll study what Java, Ruby and .Net does and see what approach makes sense. Seems like Java relies on the Augmenter, which makes sense from a Java perspective, but makes the user deal with all the details. I aim for something that is automatic but also configurable.

@j3soon
Copy link
Contributor Author

j3soon commented Aug 15, 2023

Hi @isaulv, thanks for rekindling this discussion.

Studying the implementations in other languages before proceeding seems like a promising approach. If you came across any new insights or potentially better implementations, please share them with us. I'm looking forward to your findings and would be happy to learn how to implement this in the best way possible. :)

@github-actions github-actions bot added Stale and removed Stale labels May 21, 2024
@diemol diemol added the Stale label Nov 22, 2024
@github-actions github-actions bot removed the Stale label Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants