Skip to content

[py] Update WPEWebKit support code #13278

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

Merged
merged 5 commits into from
Jan 21, 2024

Conversation

lauromoura
Copy link
Contributor

Description

This PR changes the support code around WPEWebKit helper modules after the last update, a couple of years ago.

Motivation and Context

In the recent Selenium versions, some changes in the way the driver classes are named and the move towards options instead of raw capabilities dictionaries, alongside other minor issues, made the existing WPEWebkit module to fail to create the driver correctly. For example, failing either to find the right service executable or failing the capability verification.

The WPEWebKit is a WebKit port focused on embedded devices. Developers creating WebApps for these devices can take advantage of the remote webdriver (used by these helper classes) to automate testing directly on the device.

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.
    • There's a change in a wpewebkit.Service parameter, but they're not yet covered by the API docs. Should I add them in a separate PR?
  • I have updated the documentation accordingly.
    • Ditto
  • I have added tests to cover my changes.
    • The changes should be covered by the existing tests, so no new tests added.
  • All new and existing tests passed.
    • I didn't see any failure in the unit tests and most of the //py:test-wpewebkit passes, with some specific failures that will be dealt in a separate PR.

Copy link
Member

@titusfortner titusfortner left a comment

Choose a reason for hiding this comment

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

To be honest, I think these classes should be in a separate library from Selenium. You should be able to subclass what is there and have it work properly. (If you can't, we should fix that). If it's in Selenium, it should match what the other drivers are doing.

@lauromoura
Copy link
Contributor Author

(Updated after missign a tox fix and rebase)

@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2023

Codecov Report

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

Comparison is base (b556ac3) 58.07% compared to head (d1e955b) 58.18%.
Report is 1 commits behind head on trunk.

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

Files Patch % Lines
py/selenium/webdriver/wpewebkit/webdriver.py 50.00% 2 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##            trunk   #13278      +/-   ##
==========================================
+ Coverage   58.07%   58.18%   +0.10%     
==========================================
  Files          88       88              
  Lines        5340     5330      -10     
  Branches      224      224              
==========================================
  Hits         3101     3101              
+ Misses       2015     2005      -10     
  Partials      224      224              

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

- Naming conventions, as WPEWebKit does not follow the Simple
  capitalized name scheme
- (TODO): Driver location finding
- Remove non-standard default capabilities, like in 497cde3
- Add legacy desired_capabilities to new style Options, like in 9f5801c
- To match parent Service class

Amend into Update WPEWebKit service parameter
Receiving a Service instance instead of service details spread out
@lauromoura
Copy link
Contributor Author

Pinging @titusfortner for review

@titusfortner titusfortner added this to the 4.17 milestone Jan 16, 2024
@titusfortner titusfortner merged commit 24d88d7 into SeleniumHQ:trunk Jan 21, 2024
lauromoura added a commit to lauromoura/WebKit that referenced this pull request Mar 29, 2024
https://bugs.webkit.org/show_bug.cgi?id=267836

Reviewed by NOBODY (OOPS!).

Bump selenium tests to upstream 907b2197dada02, which includes the fixes
for WPE from SeleniumHQ/selenium#13278

The changes in our runner code were minimal, basically moving the driver
name to lowercase.

* Tools/Scripts/webkitpy/webdriver_tests/webdriver_driver_wpe.py:
(WebDriverWPE.selenium_name):
* WebDriverTests/imported/selenium/common/: Updated to new revision
* WebDriverTests/imported/selenium/importer.json: Update hash to be
  imported
* WebDriverTests/imported/selenium/py: Updated to new revision
webkit-commit-queue pushed a commit to lauromoura/WebKit that referenced this pull request Apr 1, 2024
https://bugs.webkit.org/show_bug.cgi?id=267836

Reviewed by Carlos Garcia Campos.

Bump selenium tests to upstream 907b2197dada02, which includes the fixes
for WPE from SeleniumHQ/selenium#13278

The changes in our runner code were minimal, basically moving the driver
name to lowercase.

* Tools/Scripts/webkitpy/webdriver_tests/webdriver_driver_wpe.py:
(WebDriverWPE.selenium_name):
* WebDriverTests/imported/selenium/common/: Updated to new revision
* WebDriverTests/imported/selenium/importer.json: Update hash to be
  imported
* WebDriverTests/imported/selenium/py: Updated to new revision

Canonical link: https://commits.webkit.org/276878@main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants