W3C Compatability Fixes #372
Conversation
Codecov Report
@@ 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
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.
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.
@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.
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. |
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