-
Notifications
You must be signed in to change notification settings - Fork 278
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
Session autostart #733
Session autostart #733
Conversation
src/Session.php
Outdated
@@ -141,9 +144,7 @@ public function getSelectorsHandler() | |||
public function visit($url) | |||
{ | |||
// start session if needed |
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.
You can remove // start session if needed
comment from all 3 methods, because it's already present in start
method.
Updated |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment is useless. The code is clear enough (and the phpdoc already says it too)
@@ -363,6 +362,7 @@ public function wait($time, $condition = 'false') | |||
*/ | |||
public function resizeWindow($width, $height, $name = null) | |||
{ | |||
$this->start(); |
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.
technically, these actions are not defined as supported before visiting (see the phpdoc of start
)
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.
But @peterrehm is using them and they work perfectly. Otherwise there is no way to open window initially with given size so that upon page visiting we won't be resizing it again.
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 the list of Methods i the phpdoc. Is this what you meant?
@@ -83,34 +101,10 @@ public function testRestart() | |||
$this->session->restart(); | |||
} | |||
|
|||
public function testVisitWithoutRunningSession() |
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.
this test must not be removed
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.
I removed the test as I added the tests for the start() method.
As start() is tested for both options I thought it is enough to test just that start()
is called in e.g. the visit method.
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.
well, the test you kept describes the wrong interaction with the driver (it does not describe the usage of isStarted
.
and I would keep the test ensuring we can visit without starting, as the test does not need to know that the code is calling $this->start()
internally rather than directly calling the driver.
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.
But the test ensures that start is called:
public function testVisit()
{
$this->driver
->expects($this->once())
->method('start');
$this->driver
->expects($this->once())
->method('visit')
->with($url = 'some_url');
$this->session->visit($url);
}
The test with isStarted is not contained in testStartWithoutRunningSession
and testStartWithRunningSession
as the logic is moved to the start
method.
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.
But when you call $this->visit
, the isStarted
method of the driver is still called. So your description of the interaction is incomplete (and btw, because it is incomplete, isStarted
would return null
, not false
).
PHPUnit mocks don't complain about incomplete interaction specifications, which is why your test is passing (and a reason why I actually prefer Prophecy, but I haven't converted the whole Mink testsuite to it)
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.
But IMHO it doesn't matter in this case as we are finde if started() is called and afterwards the visit() method is called. Otherwise tests would need to be duplicated for the other autostart methods as well.
If you want I can add this in.
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.
@stof , @peterrehm , any updates? This seem to be last change to be made before merging.
P.S.
Change actually needs to be made on #732 instead and this PR rebased on top of it.
8b6f6c8
to
bcd5603
Compare
bcd5603
to
da11434
Compare
As discussed in #731.
Will be rebased after #732 is merged.
Tickets: