-
Notifications
You must be signed in to change notification settings - Fork 163
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
W3C Compatability Fixes #372
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #372 +/- ##
============================================
+ Coverage 89.97% 92.77% +2.79%
- Complexity 168 193 +25
============================================
Files 1 1
Lines 469 595 +126
============================================
+ Hits 422 552 +130
+ Misses 47 43 -4 ☔ View full report in Codecov by Sentry. |
tests were initially failing as |
See https://github.com/minkphp/webdriver-classic-driver for our in-progress work on implementing a new WebDriver-based driver for Mink (for now, all the initial implementation is in a PR, not yet in the main branch). |
@stof ah interesting 😸 👍 Would you please consider accepting this change into this project in the meantime? It's a small update, not disruptive to existing users, is successfully running over Drupal core's extensive tests, and will allow projects to get off > 1 year old browsers for testing without waiting for the new project to be finished and released |
.github/workflows/tests.yml
Outdated
- name: Replace instaclick/php-webdriver for new selenium versions | ||
run: | | ||
if [ "${{ matrix.selenium }}" == "latest" ]; then | ||
composer require lullabot/php-webdriver:dev-main |
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.
This published fork does not publish the fact that it replaces instaclick/php-webdriver
so composer will happily install both in parallel, with no guarantee about which one will win for defining the class. So this looks totally wrong to me.
In the current situation, I'm against merging this PR and releasing as is, because it actually makes the package unreliable for users.
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.
@stof it is marked as replacing it https://github.com/Lullabot/php-webdriver/blob/main/composer.json#L19 and I can see it doing so in the tests I just added:
https://github.com/minkphp/MinkSelenium2Driver/actions/runs/6392945083/job/17351217167?pr=372
Run if [ "latest" == "latest" ]; then
if [ "latest" == "latest" ]; then
composer require lullabot/php-webdriver:dev-main
fi
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
COMPOSER_PROCESS_TIMEOUT: 0
COMPOSER_NO_INTERACTION: 1
COMPOSER_NO_AUDIT: 1
./composer.json has been updated
Running composer update lullabot/php-webdriver
Loading composer repositories with package information
Updating dependencies
Lock file operations: 1 install, 0 updates, 1 removal
- Removing instaclick/php-webdriver (1.4.16)
- Locking lullabot/php-webdriver (dev-main 3014d58)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 1 install, 0 updates, 1 removal
- Downloading lullabot/php-webdriver (dev-main 3014d58)
- Removing instaclick/php-webdriver (1.4.16)
- Installing lullabot/php-webdriver (dev-main 3014d58): Extracting archive
Package phpunit/php-token-stream is abandoned, you should avoid using it. No replacement was suggested.
Generating autoload files
26 packages you are using are looking for funding.
Use the `composer fund` command to find out more!
I'll make a proper release of the library before this new test is merged, I just wanted to get tests etc passing here first 😄
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.
ah indeed. By default, packagist shows the info for the latest release, and this is only in the dev version.
@stof thanks for the review! I'll address the issues you've raised |
@stof ready for review! |
@justafish this looks great. It'd be nice to run Drupal's tests against firefox and other browsers at last. @stof is there scope to merge this as a stop gap until the work on https://github.com/minkphp/webdriver-classic-driver is complete? |
Hi @stof - could you please let me know if the project is willing to accept this PR so I can make a release of php-webdriver? We can move Drupal to a fork of this package if necessary, but I would prefer to continue collaborating here 😸 |
Is this PR still make sense? |
If https://github.com/minkphp/webdriver-classic-driver solves this PR needs, then probably not. |
Closing in favor of https://github.com/minkphp/webdriver-classic-driver . |
Fixes to enable compatibility with WebDriver.
Here are the modifications running against the full set of Drupal core tests: Lullabot/drupal-webdriver#2
To run against newer browsers/versions of selenium that have dropped support for json wire, a drop-in replacement library for instaclick/php-webdriver is needed (this is intended to null any disruption for existing test setups, but projects using instaclick/php-webdriver are currently stuck on selenium versions over a year old). Here's the replacement library, along with these proposed changes, run against the Drupal core tests as above: Lullabot/drupal-webdriver#1
Related drupal.org issue https://www.drupal.org/project/drupal/issues/3240792
Fixes #293