-
Notifications
You must be signed in to change notification settings - Fork 29
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
Test resize and maximize window also prior to page visit #10
Conversation
tests/Js/WindowTest.php
Outdated
{ | ||
$session = $this->getSession(); | ||
$session->resizeWindow(400, 300); | ||
$session->wait(1000, 'false'); |
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.
using the session to wait without a meaningful JS condition (i.e. one subject to change) is useless. Just use sleep
.
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.
Updated
have you tried it for existing drivers ? |
I am fast, but not that fast :) It wont be too easy, as I also need the not merged patch with the auto start from Mink. |
Actually do you have a hint what would be the easiest way to test that? |
no you don't need it, if you use the stable version of Mink (as it does not yet auto-start sessions at all, and so The easiest way to test your change is probably to run |
Mhm I tested it by submitting a PR to teh driver with the modified composer.json. https://travis-ci.org/minkphp/MinkSelenium2Driver/builds/199169919 Looks like there is an issue with the WindowMaximize but Resize works find. But the testWindowMaximize is skipped as it does not work in xvfb, I assume this
Do you have any clue how to fix that? Otherwise we could only allow resize and revert the autostart for the maximize function as it might not make any sense? /cc @aik099 |
@peterrehm the fact that maximize does not work in Selenium when using xvfb is unrelated to this topic (and is why we skip the existing maximize test in the Selenium2Driver testsuite on Travis). |
So much for the idea of quickly provide a PR for Mink :) I am struggling with Selenium etc. on my local instance. I have a lot of failing tests
So in conclusion the Any clue about the failing tests? I am using now Selenium 3 with Firefox 51.0.1 and latest geckodriver. |
Skipping tests is handled by the Config class living in the testsuite of each driver (the exact class name depends on the driver). The driver testsuite calls
GeckoDriver does not passes all tests yet. some of them are because GeckoDriver is not yet fully implemented (some APIs are missing) while some others are because we are too strict on our assertions (window name tests are in this category). |
@stof minkphp/MinkSelenium2Driver#265 Provided the adusted skip configuration. Anything else needs to be done in this case? :) |
Okay, indeed, looks much better on Chrome. Can we get the several PRs of today now merged or is there anything else? /cc @aik099 |
Looks good. But we need to be careful about merge order to avoid failing builds. @peterrehm maybe you can, in main task, specify order in which PR should be merged? |
I would merge it like this:
|
@aik099 Anything else you need? |
Nothing else. Just waiting to @stof to catch up and merge all these PRs. |
Okay, perfect! |
@stof Anything else you need? It would be great to get this merged so I have it off my desk. |
@stof Any chance to get this merged? If this is not wanted any more let me know and I can close the PRs. |
As discussed minkphp/Mink#732 and minkphp/Mink#733