Skip to content
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

Support MinkSelenium2Driver page timeout #38

Merged
merged 49 commits into from
Nov 12, 2024

Conversation

uuf6429
Copy link
Member

@uuf6429 uuf6429 commented Nov 2, 2024

Fixes #36.

@uuf6429 uuf6429 requested a review from aik099 November 2, 2024 15:25
Copy link

codecov bot commented Nov 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.40%. Comparing base (8e3c6ca) to head (2da66a7).
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main      #38      +/-   ##
============================================
- Coverage     84.61%   84.40%   -0.22%     
- Complexity      194      196       +2     
============================================
  Files             1        1              
  Lines           494      500       +6     
============================================
+ Hits            418      422       +4     
- Misses           76       78       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

aik099

This comment was marked as resolved.

@uuf6429 uuf6429 requested a review from aik099 November 2, 2024 18:39
src/WebdriverClassicDriver.php Outdated Show resolved Hide resolved
src/WebdriverClassicDriver.php Show resolved Hide resolved
@aik099 aik099 force-pushed the chore/support-old-page-timeout branch from ba4bab6 to 02da744 Compare November 3, 2024 11:04
@uuf6429 uuf6429 requested a review from aik099 November 3, 2024 11:32
tests/Custom/TimeoutTest.php Show resolved Hide resolved
tests/Custom/TimeoutTest.php Show resolved Hide resolved
@uuf6429 uuf6429 marked this pull request as draft November 3, 2024 18:19
aik099

This comment was marked as resolved.

tests/Custom/TimeoutTest.php Outdated Show resolved Hide resolved
@uuf6429

This comment was marked as outdated.

aik099

This comment was marked as resolved.

@uuf6429 uuf6429 force-pushed the chore/support-old-page-timeout branch from 0599ae5 to 7c435d2 Compare November 3, 2024 20:06
@aik099
Copy link
Member

aik099 commented Nov 4, 2024

The failed builds on the https://github.com/minkphp/webdriver-classic-driver/actions/runs/11654305762 failed due cURL request to Selenium Server took too long to respond (>3 minutes). That is actually default cURL timeout.

Looks like Selenium Server in a Docker container died.

@uuf6429
Copy link
Member Author

uuf6429 commented Nov 4, 2024

Thanks for the tries so far.

The situation is very strange:

  • it seems to happen only after setting the page timeout to 500ms multiple times
  • it applies to SE3 and SE4
  • I think it didn't crash before for Firefox
  • Couldn't reproduce it locally at all

@aik099
Copy link
Member

aik099 commented Nov 4, 2024

Thanks for the tries so far.

The situation is very strange:

  • it seems to happen only after setting the page timeout to 500ms multiple times
  • it applies to SE3 and SE4
  • I think it didn't crash before for Firefox
  • Couldn't reproduce it locally at all

Very strange indeed.

I've tried combo of TimeoutsTest and WindowTest and they've passed.

Seems like some cumulative effect takes place (e.g. tests in between TimeoutsTest and WindowTest add up to produce super long running time). Maybe we've just added more tests and some critical limit was hit.

Keep in mind, that we don't start/top Selenium Server session multiple times (e.g. Driver::start/Driver::stop), but instead do Driver::reset to close windows and clean cookies.

tests/Custom/TimeoutTest.php Outdated Show resolved Hide resolved
@aik099
Copy link
Member

aik099 commented Nov 4, 2024

It seems, that I'm getting closer. But I'm out of ideas.

I've tried removing all of the added code one by one to see if the issues will be fixed (e.g. deprecation checking, setting 500ms timeout, etc.). Nothing helped so far.

The problem always seems to be with testResizeWindow (see https://github.com/minkphp/webdriver-classic-driver/actions/runs/11663251677/job/32471294459). It does:

  1. visits the page
  2. loop for 1sec to wait for the window to be resized
  3. checks window dimensions

Page visiting fails:

Facebook\WebDriver\Exception\Internal\WebDriverCurlException: Curl error thrown for http POST to /session/de256684ffe02744efbebfa2205ed1b8/url with params: {"url":"
http://host.docker.internal:8002/index.html
"}
   ├                                                                                                                                                                                                                  
   ├ Operation timed out after 180000 milliseconds with 0 bytes received                                                                                                                                              

The fact that we saw that issue while developing timeout-changing code seems like a coincidence.

@uuf6429
Copy link
Member Author

uuf6429 commented Nov 4, 2024

The problem always seems to be with testResizeWindow

Not always, sometimes it fails after and sometimes before - I noticed that when we had the larger build matrix - but it always seems to break somewhere there, which is interesting considering the amount of other successful tests between the timeout test and the window test.

The fact that we saw that issue while developing timeout-changing code seems like a coincidence.

But nonetheless it has to be related to the timeout tests, right? Everything worked fine without the deprecation test I added in this PR.


I noticed the GH action you added...that looks very interesting for debugging. 👍

@aik099
Copy link
Member

aik099 commented Nov 4, 2024

I noticed the GH action you added...that looks very interesting for debugging.

You're welcome to use it. I can't proceed any further without real-time debugging with xDebug. Trying to arrange it with https://github.com/nektos/act .

@aik099
Copy link
Member

aik099 commented Nov 4, 2024

I've managed to start https://github.com/nektos/act, but all tests are passing. Seems, that issue can only reproduced in the GitHub Actions.

I've also used a built-in PHP 8.3 web server and it's passing locally.

Maybe I'll try to use a built-in PHP web server from a different PHP version on GitHub Actions. We had some issues with this in the past.

@uuf6429
Copy link
Member Author

uuf6429 commented Nov 4, 2024

@aik099 thanks for the investigation. I'll give it a look myself now.

@uuf6429
Copy link
Member Author

uuf6429 commented Nov 4, 2024

@aik099 I found the solution and possibly the problem, it seems to be because we do not really run headless in the workflow. After the necessary changes, the test passes without a hitch (and is also quite fast maybe not, it was just a few seconds faster): https://github.com/minkphp/webdriver-classic-driver/actions/runs/11670274546/job/32494122266

I copied that config from another project, the main noteworthy things is that (1) the docker service does not start xvfb (true headless), (2) the config requests that chrome is started headless and (3) config also requests chrome not to use the gpu.


On a related subject, as a Behat user, this thing about properly configuring headless has always been a bit of a hassle. Even the slightly wrong config causes crashes and extremely hard to understand errors. Since this driver takes care of some defaults settings, I'm wondering if it may make sense to bake this in to make it easier for the end users. The problem is how do we pass this information ($headless = true) from mink?

One possible way is to pass a non-standard capability (e.g. "headless") which we then handle inside the driver. What do you think?

@aik099
Copy link
Member

aik099 commented Nov 4, 2024

@uuf6429 , since in all cases it's a timeout to the local web server (started from bootstrap.php) ocurred, then I've decided to run that web server from GitHub Actions directly. The issue I'm facing is that tests can't connect to http://host.docker.internal:8002 at all.

I have no idea how to fix this. Please help.

@uuf6429
Copy link
Member Author

uuf6429 commented Nov 4, 2024

The issue I'm facing is that tests can't connect to http://host.docker.internal:8002 at all.

Hmm, I'm not sure... maybe try MINK_HOST: 0.0.0.0:8002 (instead of localhost)?

But in any case, check my previous message - there shouldn't be any difference in how the web server is started. Also selenium logs say that the timeout occurred when trying to control the browser to visit the fixture url, so it is not necessarily the problem of the web server (if the browser is already crashed/stuck, we will get the curl error).

@aik099
Copy link
Member

aik099 commented Nov 4, 2024

@aik099 I found the solution and possibly the problem, it seems to be because we do not really run headless in the workflow. After the necessary changes, the test passes without a hitch (and is also quite fast maybe not, it was just a few seconds faster): https://github.com/minkphp/webdriver-classic-driver/actions/runs/11670274546/job/32494122266

I copied that config from another project, the main noteworthy things is that (1) the docker service does not start xvfb (true headless), (2) the config requests that chrome is started headless and (3) config also requests chrome not to use the gpu.

On a related subject, as a Behat user, this thing about properly configuring headless has always been a bit of a hassle. Even the slightly wrong config causes crashes and extremely hard to understand errors. Since this driver takes care of some defaults settings, I'm wondering if it may make sense to bake this in to make it easier for the end users. The problem is how do we pass this information ($headless = true) from mink?

One possible way is to pass a non-standard capability (e.g. "headless") which we then handle inside the driver. What do you think?

Here are my configs (in desired capabilities) for headless testing:

'moz:firefoxOptions' => array(
    'args' => array(
        '--headless',
        '--width=1550',
        '--height=1160',
    ),
),
'goog:chromeOptions' => array(
    'args' => array(
        '--headless',
        // '--window-size=1550,1160', // doesn't work
        '--start-maximized', // works
    ),
),
  1. we can look at '--headless' element presence in the options sub-array in desired capabilities for a used browser and if found add other args as needed (if they're not provided already)
  2. I would keep headless disabled by default to allow developers to see what tests are doing in the web browser
  3. I've noticed, that Firefox also isn't executed in the headless mode and maybe it should
  4. since the XVFB is disabled now, then maybe we can enable testWindowMaximize again (excluded currently due to XVFB usage on GitHub Actions with Maximizing the window does not work when running the browser in Xvfb. message)

P.S.
I've removed the debug code, but left these changes:

  1. don't enforce default timeouts from the driver side, because we're restarting the session (in setUp) and Selenium Server default timeouts would kick in on their own
  2. using named datasets in the deprecatedPageLoadDataProvider method

@aik099 aik099 force-pushed the chore/support-old-page-timeout branch from b25b3b2 to 2c6a20c Compare November 12, 2024 08:13
@aik099
Copy link
Member

aik099 commented Nov 12, 2024

Rebased on main branch after #47 merge.

@uuf6429
Copy link
Member Author

uuf6429 commented Nov 12, 2024

@aik099 hey, it seems the crash is also happening for firefox/selenium2:
https://github.com/minkphp/webdriver-classic-driver/actions/runs/11803930387/job/32883023966?pr=38
I guess that build was cancelled in your last changes, so we never saw this happening:
https://github.com/minkphp/webdriver-classic-driver/actions/runs/11793213060/job/32848310340

We could also skip this case, but the situation (combination of browser with selenium) is in general very odd.

@aik099
Copy link
Member

aik099 commented Nov 12, 2024

The Selenium2+Firefox wasn't crashing all the time it seems (see https://github.com/minkphp/webdriver-classic-driver/actions/runs/11679073840/job/32519553552). Very odd.

I wonder why it's happening on GitHub Actions only with Selenium Image involved. I've tried using Act tool to run that build locally and it never failed for window tests.

Anyway please add the same exclusion for the Firefox+Selenium2 case as I did for non-Firefox versions. And hopefully we can merge this long-running PR.

@uuf6429
Copy link
Member Author

uuf6429 commented Nov 12, 2024

Sorry @aik099 I just decided to exclude the firefox d&d case in this PR, just to get rid of it quick and finally have a green check for tests (and since that part can reuse some code from this PR).

Closes #49.

@aik099
Copy link
Member

aik099 commented Nov 12, 2024

Sorry @aik099 I just decided to exclude the firefox d&d case in this PR, just to get rid of it quick and finally have a green check for tests (and since that part can reuse some code from this PR).

Do as you see fit. Don't forget to close the #49 mentioning that fact. 😉

@uuf6429 uuf6429 merged commit ca55cde into main Nov 12, 2024
31 of 32 checks passed
@uuf6429 uuf6429 deleted the chore/support-old-page-timeout branch November 12, 2024 21:31
@aik099
Copy link
Member

aik099 commented Nov 13, 2024

@uuf6429 , when merging anything (especially using squash) please adjust the commit message. We have cleaner git log with fewer commits now, but these commits have very unclean commit messages, like ca55cde .

@uuf6429
Copy link
Member Author

uuf6429 commented Nov 13, 2024

@aik099 isn't that just the description? Or did you mean the commit message (that it mentions MinkSelenium2Driver)?

https://github.com/minkphp/webdriver-classic-driver/commits/main/

I could also just remove the description, it's also fine by me.

@aik099
Copy link
Member

aik099 commented Nov 13, 2024

Yeah, I meant the commit message itself, which explains how we've got to the code we have, that doesn't matter anymore:

Support MinkSelenium2Driver page timeout (https://github.com/minkphp/webdriver-classic-driver/pull/38)
* Support MinkSelenium2Driver page timeout

* Trigger deprecation of old-style page timeout

* Replace duplicate code with comment

* Test deprecations

* Rework custom tests to not depend on magic setup

* Set up standard defaults to avoid contaminated sessions

* [debug] Only run tests that fail

* [debug] Adjusted PHPUnit 10 test filtering expression to work on PHPUnit 9

* [debug] remove irrelevant (for debugging) build config sections to speed it up

* [debug] Reduce matrix size even more

* Don't let driver define Selenium Server default timeouts

* [debug] confirm, that timeout tests executed at the beginning would break window tests down the road

* Revert "[debug] confirm, that timeout tests executed at the beginning would break window tests down the road"

This reverts commit https://github.com/minkphp/webdriver-classic-driver/commit/b33289bebd9138dd5394b3c4c80e334db9f23b28.

* [debug] see if Selenium actually sets timeouts as needed

* Don't reset timeout at the test end, because new driver instance is created in setup

* [debug] PhpStan fix

* [debug] see if session with incorrect timeout is shared between tests

* Fixed formatting error

* [debug] see if deprecation catching causes timeout issues

* [debug] see of sleepy page causes a timeout issue

* [debug] change data provider declaration of the timeouts test

* [debug] removed debug output in the driver code, because not timeouts are causing the issue with window test

* [debug] Look inside GitHub Actions with an SSH session

* [debug] start web server outside of PHPUnit bootstrap file

* [debug] remove unrelated build config parts

* [debug] corrected host of locally started mink server

* [debug] corrected hostname for started web server checking

* [debug] another try to get local web server running

* [debug] reduce test count to figure out why web server is not working

* [debug] Experiment with WEB_FIXTURES_HOST env var detection

* [debug] another try to debug what URL is actually used by test suite

* [debug] another try

* Allow overriding env vars from "phpunit.xml.dist" via env vars from GitHub Actions

* [debug] removed some of the debug code

* [debug] change local web server detection code to avoid it's crashing in the checking process

* [debug] try to make web server available for Selenium Server

* [debug] undo experimental port mapping

* [debug] attempt to start web sever on all ips

* [debug] rolled back CI code that reduced build count

* [debug] rolled back the debug code, that started web server outside of PHPUnit code

* rolled back changes to "phpunit.xml.dist" (to be discussed in other issue)

* Partially skip short "pageLoad" timeout test in Google Chrome

* Fully skip short "pageLoad" timeout test in Google Chrome

* Only skip test added in this PR for Google Chrome

* Apparently "chromium" and "edge" browsers also freeze, when setting "pageLoad" timeout several times. So skip test in there as well.

* Exclude timeout deprecation test only on GitHub Actions

* Minor test fixes and improvements

* Disable test on old firefox

* Fix https://github.com/minkphp/MinkSelenium2Driver/issues/391

---------

Co-authored-by: Alex <[email protected]>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for page load and pageLoad timeout type
3 participants