search

minkphp / Mink

PHP web browser emulator abstraction
https://mink.behat.org/
MIT License
1.6k stars 280 forks source link

Auto-start session only upon 1st "->visit(...)" method call breaks my test suite #731

Open peterrehm opened 7 years ago

peterrehm commented 7 years ago

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)
    │
aik099 commented 7 years ago

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.

peterrehm commented 7 years ago

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

aik099 commented 7 years ago

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:

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.

stof commented 7 years ago

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

stof commented 7 years ago

or we can indeed handle it at the Session level, solving it for all drivers

peterrehm commented 7 years ago

Shall I submit a PR for the autostart and the session handling? So just silently do not restart it if already started?

stof commented 7 years ago

yeah, please submit a PR

aik099 commented 7 years ago

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:

peterrehm commented 7 years ago

Okay, both PRs are provided.

yakobe commented 7 years ago

Any update on this?

peterrehm commented 7 years ago

@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?

yakobe commented 7 years ago

@peterrehm pretty much the same issues as you with the window resize. I saw the PR's but both are stalled at the moment.

peterrehm commented 7 years ago 
    /**
     * @BeforeScenario
     */
    public function beforeScenario()
    {
        if ($this->getSession()->getDriver() instanceof Selenium2Driver) {
            $this->getMink()->getSession()->start();
            $this->getSession()->resizeWindow(1440, 900, 'current');
        }
    }
jakzal commented 7 years ago

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).

aik099 commented 7 years ago

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

peterrehm commented 7 years ago

Btw how about this and the related PRs? Shall I close them?

aik099 commented 7 years ago

If the fix was created/merged, then issue/pr will be closed automatically.

peterrehm commented 7 years ago

I know, but as there is no activity in regards to the PRs. I can close them if not wanted.

aik099 commented 7 years ago

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?

peterrehm commented 7 years ago
jakzal commented 7 years ago

@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.

aik099 commented 7 years ago

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.

aik099 commented 7 years ago

https://github.com/minkphp/Mink/issues/731#issuecomment-313844707

I've reviewed all PRs and:

peterrehm commented 7 years ago

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.

jakzal commented 7 years ago

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?

aik099 commented 7 years ago

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.

jakzal commented 7 years ago

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().

aik099 commented 7 years ago

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.

func0der commented 4 years ago

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.

stof commented 1 year ago

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)