Only start session if the session has not been started before #732
Conversation
| @@ -83,34 +101,10 @@ public function testRestart() | |||
| $this->session->restart(); | |||
| } | |||
|
|
|||
| public function testVisitWithoutRunningSession() | |||
There was a problem hiding this comment.
this test must not be removed. We still need to cover the case of calling visit on a started and on a non-started session IMO (thus, your mocks don't describe the actual interaction happening, as you removed the mocking of isStarted
There was a problem hiding this comment.
Actually the discussion from https://github.com/minkphp/Mink/pull/733/files#r99773293 is meant to happen on this PR instead.
src/Session.php
Outdated
| @@ -67,7 +67,10 @@ public function isStarted() | |||
| */ | |||
| public function start() | |||
| { | |||
| $this->driver->start(); | |||
| // start session if needed | |||
There was a problem hiding this comment.
useless comment. The phpdoc already says it
src/Session.php
Outdated
| * | ||
| * Calling any action before visiting a page is an undefined behavior. | ||
| * The only supported method calls on a fresh driver are | ||
| * - resizeWindow() | ||
| * - maximizeWindow() |
There was a problem hiding this comment.
if we want to allow this, we must add new tests in the driver testsuite calling these methods before visiting a page, to ensure it actually works in all drivers
There was a problem hiding this comment.
btw, this does not seem related to this PR, but to #733
There was a problem hiding this comment.
Correct, that is my bad. Where would you add the tests? Looking at the CoreDriverTest and the Tests in the Selenium driver I could not see any tests on e.g. the visit method
There was a problem hiding this comment.
the driver testsuite lives in https://github.com/minkphp/driver-testsuite and is a dev dependency in all drivers repositories, to share the same testsuite in all of them.
There was a problem hiding this comment.
That makes sense. I was scanning the testsuite, I could not find a test where the autostart is tested e.g. for the visit method by grepping te repository. Do you know if there is anything in this regard?
There was a problem hiding this comment.
Allowing to call these methods without visiting a page is not related to auto-starting. It is a different feature (and this is what you need to test).
Auto-starting is not tested in the driver testsuite, because drivers don't care about it. It is implemented at the session level. But being able to resize before visiting is something that drivers themselves must support
There was a problem hiding this comment.
Oh okay. So what do you think about: minkphp/driver-testsuite#10
This reverts commit d3a2a1b.
As discussed in #731.