Mink
Mink copied to clipboard
minkphp
Auto-start session only upon 1st "->visit(...)" method call breaks my test suite
PR #705 from @aik099 breaks my testsuite as I am resizing the Window prior to each scenario.
class FeatureContext extends RawMinkContext
{
use KernelDictionary;
/**
* @BeforeScenario
*/
public function beforeScenario()
{
if ($this->getSession()->getDriver() instanceof Selenium2Driver) {
$this->getSession()->resizeWindow(1440, 900, 'current');
}
}
}
This breaks with the error:
┌─ @BeforeScenario # AppBundle\Features\Context\FeatureContext::beforeScenario()
│
╳ Fatal error: Call to a member function window() on null (Behat\Testwork\Call\Exception\FatalThrowableError)
│
That is unexpected.
Any other methods, except resizeWindow that can use session before ->visit(...) is called? We can auto-start session in them as well. I hope there are no much such methods.
I am not using others, the question is if there are other. As we cannot know what others want to do prior the forst visit call I would suggest then leave it as it is but maybe optimize the session start prozedure.
Right now if you call session start with the selenium driver twice you get different sessions. Only if I add the check if the session is started and start it conditionally it works as expected.
How about throwing an exception when you call start on an already started session or only start if it has not been started previously as you are doing here: https://github.com/minkphp/Mink/pull/705/files#diff-06f6ce27521fd9b588fa1d23c8ad02d1R143
I am not using others, the question is if there are other. ...
Here are the list of methods (your's included), that might also need to auto-start session:
-
resizeWindow -
maximizeWindow
How about throwing an exception when you call start on an already started session or only start if it has not been started previously
We can do that probably. Also when attempting to stop non-started session the exception can be thrown. Nobody really experienced issues with that before by the way.
Right now if you call session start with the selenium driver twice you get different sessions.
I would consider it a bug in the selenium driver. It should not start a second time if it is already started
Shall I submit a PR for the autostart and the session handling? So just silently do not restart it if already started?
or we can indeed handle it at the Session level, solving it for all drivers
Yes, on session level. That's what I was talking about.
Shall I submit a PR for the autostart and the session handling?
That would be 2 PRs:
- one for auto-starting session during resize/maximize
- another one for not starting/stopping session when already started/stopped
@yakobe See in the PR's, both are awaiting to be merged. You can meanwhile fix that in your CI suite by manually starting the session. Or what issues do you have?
@peterrehm pretty much the same issues as you with the window resize. I saw the PR's but both are stalled at the moment.
/**
* @BeforeScenario
*/
public function beforeScenario()
{
if ($this->getSession()->getDriver() instanceof Selenium2Driver) {
$this->getMink()->getSession()->start();
$this->getSession()->resizeWindow(1440, 900, 'current');
}
}
This change also broke the page object extension (see https://github.com/sensiolabs/BehatPageObjectExtension/issues/92#issuecomment-313468134), where in a class extending Mink's Page we do:
public function open(array $urlParameters = array())
{
$url = $this->getUrl($urlParameters);
$this->getDriver()->visit($url);
$this->verify($urlParameters);
return $this;
}
So session is never initialised as we call the driver directly (we used to access Session here, but getSession is deprecated now).
https://github.com/minkphp/Mink/issues/731#issuecomment-313721610
@jakzal , what you've posted already is part of Page class: https://github.com/sensiolabs/BehatPageObjectExtension/blob/master/src/PageObject/Page.php#L56
I know, but as there is no activity in regards to the PRs. I can close them if not wanted.
In FOSS world delays is usual thing, because FOSS isn't repo maintainer's primary work place 😉 . Not merged PR isn't PR that nobody wants to see.
Could you please list all associated PRs from this and other repos (e.g. driver repos) so that I can bulk review them once more and see if anything needs to be fixed before merging?
- https://github.com/minkphp/MinkSelenium2Driver/pull/265
- https://github.com/minkphp/Mink/pull/732
- https://github.com/minkphp/Mink/pull/733
- https://github.com/minkphp/driver-testsuite/pull/10
@jakzal , what you've posted already is part of Page class:
Yes, that's what I said. We introduced this code to avoid using deprecated getSession. Mentioned PR breaks the extension.
https://github.com/minkphp/Mink/issues/731#issuecomment-313873363
I can't seem to find previous code in PageObject that was causing the issues. The only commit I've found was changing $this->getSession()->visit( to $this->getDriver()->visit(. Since visit method is doing auto-starting all should have worked before as well.
https://github.com/minkphp/Mink/issues/731#issuecomment-313844707
I've reviewed all PRs and:
- https://github.com/minkphp/MinkSelenium2Driver/pull/265 - merged it
- https://github.com/minkphp/Mink/pull/732 - same change as below is in discussion
- https://github.com/minkphp/Mink/pull/733 - one unresolved discussion in there, that actually should be done in https://github.com/minkphp/Mink/pull/732
- https://github.com/minkphp/driver-testsuite/pull/10 - approved, but according to merge order it should be merged last
If I recall correctly this introduced the issue: https://github.com/minkphp/Mink/commit/acf5fb1ec70b7de4902daf75301356702a26e6da
And the issue is the session gets restarted without the checks.
I can't seem to find previous code in PageObject that was causing the issues. The only commit I've found was changing $this->getSession()->visit( to $this->getDriver()->visit(. Since visit method is doing auto-starting all should have worked before as well.
Driver's visit() doesn't trigger session auto-start at the moment?
Driver's visit() doesn't trigger session auto-start at the moment?
It does.
Before session was auto-started in getSession and not it's auto-started in visit, so the ->getSession()->visit() code should have worked in either case.
Since then code was change to ->getDriver->visit() and it would only work after https://github.com/minkphp/Mink/commit/acf5fb1ec70b7de4902daf75301356702a26e6da change, which isn't released.
Driver's visit() doesn't trigger session auto-start at the moment? It does.
Could you point me to relevant code?
acf5fb1 is precisely what breaks the page object extension. Session is not being initialised as we're not calling Session's visit().
Ah, I get it now.
The Session::visit method (see https://github.com/minkphp/Mink/blob/master/src/Session.php#L143-L146) is auto-starting session before calling DriverInterface::visit. Since you're using driver directly now (not through session) no auto-starting happens.
I've checked all pending PRs and none, once merged, would solve the problem for you.
I highly recommend using session. Yes I know it won't be available since Mink 2.0 release, but I will get exact same problem (with session not available in https://github.com/qa-tools/qa-tools, which is also PageObject pattern implementation.
This was fixed and is now suprisingly broken again since 1.8.0 (should have maybe been the reason for a BC release). So we might as well close this bug.
The solution is: Overwrite your getSession() to start the session if not already started.
to me, the root cause of the issue is that the library extends the Mink DocumentElement instead of wrapping the Mink API into its own API (which would allow it to keep using the session API)