search

minkphp / webdriver-classic-driver

Mink driver for the W3C WebDriver Classic protocol
MIT License
3 stars 5 forks source link

Implement the driver #1

Closed uuf6429 closed 11 months ago

uuf6429 commented 1 year ago

Remaining Tasks

codecov[bot] commented 1 year ago

Welcome to Codecov :tada:

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered :open_umbrella:

uuf6429 commented 1 year ago

@stof I'm guessing this PR's jobs are failing due to this deprecation notice:

image

(there are other deprecation notices)

This is one of the things that confused me originally - why does the bridge fail for my branch, but passes for MinkSelenium2Driver? As far as I am aware, the relevant code is the same and both projects are not defining any bridge config.

Additionally, I cannot even fix that deprecation since mixed is available since PHP 8.

By the way, is there any way to make the bridge somehow more explicit that it's causing the unsuccessful exit? As it is now, it's really not obvious.

stof commented 1 year ago

@uuf6429 the DebugClassLoader triggers a deprecation only for classes that are not in a common namespace. Due to the way namespaces were defined for the older drivers (which is a legacy from the fact that they were initially in the main Mink repo and they kept their FQCN when splitting them), DebugClassLoader skips those deprecations for Selenium2Driver. The reasoning for that is that for classes that have the same namespace, you will add the typehint in both at the same time (reusing the same namespace in separate composer packages is against the best practices).

Regarding not being able to fix it due to the min PHP version, if you read the deprecation message again, you will see that DebugClassLoader will also stop reporting the deprecation if you have an explicit @return mixed in your phpdoc (not relying on the parent phpdoc) as it will expect you to add that as a native type in a later major version. an alternative would be to make this new driver require PHP 8+ instead of 7.4+. When at look at the installation stats of BrowserKitDriver (the only driver for which we made a new major version for which projects had to update their requirements instead of just benefiting from semver ranges), the v2 has 90.5% of PHP 8+ for the versions used. To me, it seems likely that projects that would do the effort to migrate to a new driver will be on a maintained PHP version (which is not the case of PHP 7.4)

uuf6429 commented 1 year ago

After the latest iteration, most of the tests are passing 💪

One particular failure is from chrome-selenium-2 \Behat\Mink\Tests\Driver\Js\WindowTest::testResizeWindow test. When the test is run in isolation it always works, but when the whole class is run, it's always failing.

Adding $this->getSession()->restart() to the beginning of the test seems to fix it.

If I understand the situation correctly, the previous tests are opening (but not closing) some popups, and in some case, the popup is left as the main window. When this test runs and tries to resize the popup, it's not able to do that and therefore it fails the size check. (at least this is my guess)

  1. is it ok to just restart the session for this test? it feels like it's only hiding the main problem
  2. is it normal that there leftover popups from previous tests?
  3. session/driver reset() does not solve the problem, but I also noticed that the DriverInterface doesn't say that existing windows should be closed
stof commented 1 year ago

Closing the main window should not be needed during reset (after all, the whole point of reset is to be faster than a restart by avoiding to close and reopen the browser). but looking at the way the initial window is handled in the driver implementation, reset should probably switch back to it if the current window is not that one.

stof commented 1 year ago

@uuf6429 I just merged a few new tests in the driver-testsuite about error handling for invalid usages of setValue (passing the wrong type of value depending on the kind of fields). Those are the ones that failed in the latest run of the CI.

stof commented 1 year ago

As https://github.com/minkphp/Mink/pull/856 has been merged, you can now use native parameter types in the driver public methods (except for $value in setValue, $user in setBasicAuth and $char in key* methods as those arguments would require using a union type and so they don't have a type yet in Mink). This requires making Mink 1.11 the minimum requirement, but this is already the case in this PR.

uuf6429 commented 1 year ago

Hey, I've been thinking regarding setBasicAuth, I don't fully understand how that should behave, which is largely why I left it as unsupported. I think there is actually a capability to fill in the basic auth modal, is this(implementing it) something we should consider? (even if for later, I would keep a note on it)

stof commented 1 year ago

Selenium2Driver does not support setting basic auth. I think this new driver cannot support it either (unless Webdriver added a feature to allow that since that time, but I don't think so).

uuf6429 commented 1 year ago

@minkphp/webdriver-classic hey, I created separate tasks/issues for the rest of the suggestions.

What's next? Is there anything else missing? I think the main point is to get the rest of the new tests merged.

aik099 commented 1 year ago

What's next? Is there anything else missing? I think the main point is to get the rest of the new tests merged.

@uuf6429 , I see, that some discussions on PR code are still unresolved. Please address them.

uuf6429 commented 1 year ago

What's next? Is there anything else missing? I think the main point is to get the rest of the new tests merged.

@uuf6429 , I see, that some discussions on PR code are still unresolved. Please address them.

@aik099 I replied to all of them. From my perspective the main remaining point is to have all the relevant PRs in WebDriverTests merged to be sure the driver is up to spec. There are some additional tasks, which I opened new GH issues for and which won't make it into the first alpha version.

If all your concerns have been addressed, please approve the PR instead of waiting for someone else to make their first move (that applies to everyone).

stof commented 1 year ago

@uuf6429 any chance to fix the CI jobs that are failing ?

uuf6429 commented 1 year ago

@uuf6429 any chance to fix the CI jobs that are failing ?

@stof if I remember correctly, they're blocked by https://github.com/minkphp/driver-testsuite/pull/84, https://github.com/minkphp/driver-testsuite/pull/75 (maybe?) and https://github.com/uuf6429/webdriver-classic-driver/pull/2 (eventually, I guess).