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

MinkPhpWebDriver is ready(ish)! #366

Closed
uuf6429 opened this issue Jun 8, 2023 · 5 comments
Closed

MinkPhpWebDriver is ready(ish)! #366

uuf6429 opened this issue Jun 8, 2023 · 5 comments

Comments

@uuf6429
Copy link
Member

uuf6429 commented Jun 8, 2023

@stof @aik099 I've got the newly-built driver passing all (24!) tests*!

While there are definitely some final changes required, I think it's in a good state to start a discussion/review on moving forward.

Here's the PR to my own repository: https://github.com/uuf6429/MinkPhpWebDriver/pull/1 (you can still see what changed)

The good parts

  • supports Selenium 2, 3 and 4
  • tested on Chrome and Firefox, and PHP 7.4-8.2
  • bridges webdriver protocol incompatibilities, such as the case of window names vs window handles
  • more functionality is tested, compared to MinkSelenium2Driver
  • can be run through act
  • fixture test server is now part of the test boostrap, so no need to care about starting it up
  • tonne of other minor improvements:- syn getter script, license file, smaller composer archive, etc

The questionable parts

  • I removed the changelog file (I wasn't thinking of contributing the driver to this project at that point, and I think it's pointless to have a changelog when github has clear release notes)
  • similarly, some coding style was also changed for the same reason
  • it now contains IDE-specific PHP attributes (which would have been more useful in the driver interface), but they are otherwise harmless
  • requires PHP 7.4 (a requirement of php-webdriver)
  • there's a leftover TODO.md which I'll make sure to remove soon
  • I removed/disable the symfony phpunit bridge thing - I found it to cause more trouble rather than be useful
  • to be able to run the tests, I had to make it run against a fork of driver-testsuite ('coz of Test was not remapping remotely-used fixture file driver-testsuite#67 and Fix tests failing from format ambiguities driver-testsuite#68)

So how shall we proceed? Do you see this as a "v2" of this repo, or as a different repo altogether? What are the main things you'd rather see changed/fixed? (what are the next steps?)


* events are not working well in Chrome + Selenium 2 (it seems to be a browser problem), but it's not new: I am able to prove that here, by having a fork testing against chrome instead of firefox.
Because of this problem, chrome+se2 tests are allowed to fail. Would be nice to have a proper fix, but I'm not sure this is possible.

@stof
Copy link
Member

stof commented Jun 9, 2023

this should not be contributed to this project anyway. It should be a separate repo.

I suggest name the package mink/webdriver-classic-driver and using Mink\Driver\WebdriverClassicDriver as namespace.
I'm using Webdriver Classic here because of the upcoming Webdriver Bidi spec of the W3C so Webdriver alone is not enough.

  • I removed/disable the symfony phpunit bridge thing - I found it to cause more trouble rather than be useful

please don't do that. The bridge ensures that the CI fails when you use deprecated APIs, which we want in our CI because we don't want our users to let us know when we are not deprecation-free.

  • it now contains IDE-specific PHP attributes (which would have been more useful in the driver interface)

given that users are not expected to use those methods directly anyway (the driver is the low-level API), I would remove those PHPStorm-specific attributes (and anyway, they would probably not help for actual usages if they are not at the interface level anyway)

@stof
Copy link
Member

stof commented Jun 9, 2023

I will create a repo for you to submit it.

@stof
Copy link
Member

stof commented Jun 9, 2023

@uuf6429 the new driver repo is at https://github.com/minkphp/webdriver-classic-driver. Please submit your work there

@oleg-andreyev
Copy link
Contributor

@uuf6429 been there done that (#304), I'd propose join forces and work on https://github.com/oleg-andreyev/MinkPhpWebDriver

@uuf6429
Copy link
Member Author

uuf6429 commented Jun 10, 2023

please don't do that. The bridge ensures that the CI fails when you use deprecated APIs, which we want in our CI because we don't want our users to let us know when we are not deprecation-free.

Typically I prefer enabling display-errors + full error-level on dev (and beStrictAboutOutputDuringTests in phpunit). I didn't like how the bridge thing is behaving way too magically (also in how it hijacks phpunit somehow), arbitrary failing test suites etc. Plus it feels like it is more of a symfony feature, for symfony devs.

given that users are not expected to use those methods directly anyway (the driver is the low-level API), I would remove those PHPStorm-specific attributes (and anyway, they would probably not help for actual usages if they are not at the interface level anyway)

Some of them are already useful within the driver. I'll keep them for now, at least until the interface part is fixed (but before first release).


I'm closing this issue.

The rest of the work goes into the new repo and @oleg-andreyev we can continue the discussion elsewhere. 👍

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

No branches or pull requests

3 participants