oleg-andreyev / MinkPhpWebDriver

Webdriver driver for Mink framework
MIT License
8 stars 9 forks source link

Allow reopened closed windows to be switched to #8

Closed andrewnicols closed 3 years ago

andrewnicols commented 4 years ago

The workaround to support firefox window switching means that the list of windows can become stale.

When a window is opened, closed, and then reopened the list of windows then contains two windows with the same name, with the currently open window likely being later in the list, for example:

Array
(
    [4294967304] => questionpreview
    [4294967307] => questionpreview
)

The switchToWindow code only looks for the first match, which is the stale window which has since been closed.

Ideally there should be no state stored here, but given the geckodriver bug that this workaround addresses, the state must be updated more aggressively.

I'm really not keen to see that the window names are checked in the click() function, and that the click causes the active window to change in order to do this. That's a load of bugs waiting to happen. Thankfully the upstream bug requiring this has recently been fixed (https://bugzilla.mozilla.org/show_bug.cgi?id=1519335) so we should be able to define a minimum supported version and remove this hack.

oleg-andreyev commented 4 years ago

@andrewnicols webdriver-1.8 was merged into "main" branch, please rebase your branch. I'll review it by the end of this week.

andrewnicols commented 4 years ago

Hi @oleg-andreyev ,

You may have merged this branch into a new main branch, but you haven't pushed that branch. It does not exist on this repository.

andrewnicols commented 4 years ago

I've rebased this on master, which is where the webdriver-1.8 branch was recently merged into.

andrewnicols commented 3 years ago

Hi @oleg-andreyev,

Any chance of a review of this? I'm trying to clear the final blockers so that I can finally move to Facebook\Webdriver and this is one of them.

Thanks

oleg-andreyev commented 3 years ago

@andrewnicols please rebase

andrewnicols commented 3 years ago

Done - thanks.

andrewnicols commented 3 years ago

Rebased, but it looks like gh-actions are all broken unrelated to this change (and my others).

oleg-andreyev commented 3 years ago

@andrewnicols I've rebased your PR once again, ignore chrome failure "Timed out receiving message from rendered", but please take a look at "WindowNameTest::testReopenWindow" as it's failing.

andrewnicols commented 3 years ago

I’m not seeing any failures there on the Github action, and I’m getting php unit errors when I try to run locally at the moment.

oleg-andreyev commented 3 years ago

I’m not seeing any failures there on the Github action, and I’m getting php unit errors when I try to run locally at the moment.

It's randomly failing with "Timed out receiving message from renderer" in 50% , yesterday I think I figured it out, previous test is resizing window window to smaller one and never restored back, so I've updated reset method .

oleg-andreyev commented 3 years ago

I'm currently thinking to skip that test for Chrome, because of time spent of it without success.

andrewnicols commented 3 years ago

As requested I've moved this all to switchWindow(). Only failure is random.

oleg-andreyev commented 3 years ago

@andrewnicols Thanks 🙏 merged