Avoid unnecessary front page requests in browser tests

Are you using xdebug.start_with_request = trigger? Because ddev uses on by default which I don't think requires the cookie. I'd like to not break the trigger mode as we use that a lot outside of ddev.

We could at least only do the request when xdebug is enabled if it's only really for that?

Based on #5 should this be NW or what's next steps?

Tried this out with a few scenarios, comments/results below:

• With xdebug.start_with_request = trigger we still need to call $session->visit() for WebDriver tests
• This works fine with the druplicon
• It's odd that this was added to BrowserTestBase, it's only needed for WebDriverTestBase
• The bug that the comment points to (in #4) was marked fixed in 2014, two years before this appeared in core The bug is to add documentation for this scenario.

I think we need to at least rename initFrontPage to something like initCookieDomain, and optionally remove it from BTB and only include it in WDTB. Otherwise think it's fine.

I guess we could deprecate it then if we want to rename it?

A small number of contrib tests call this so not sure we can just rename it: https://search.tresbien.tech/search?q=initFrontPage

I guess we can deprecate it then

I managed to open a duplicate at #3616208: Change initFrontPage() to visit robots.txt.

However that duplicate made me realise the jsonapi test here is real rather than random. I've rebased and pushed a commit that adds a front page request to that test method, and opened a follow-up issue to look into it properly #3616376: Remove front page warming step in UserTest::testCollectionFilterAccess - it's not making the test any worse, just more explicitly dependent on whatever side-effect it relies on.

Also created a stub CR here: https://www.drupal.org/node/3616377

Started to look at the deprecation but it was a bit more fiddly than I expected, so decided it was better to get to a fully green run before doing anything for that.

WebdriverTestBase will need to override ::setUp() and call its new method (whatever we call it) if we move it there out of BrowserTestBase. I think it's probably worth moving it out to save the 20ms or whatever it will be requesting the image on every test method in functional tests 20ms x hundreds is still a few seconds.

Had a go at the deprecation:

- deprecated ::initFrontPage()
- removed the call to it from BrowserTestBase
- moved the logic + comments to ::initMink() in WebDriverTestBase

One functional js test passed, see if the rest do.

OK one test was calling ::initFrontPage() for no reason twice.

Two tests were making assertions based on the front page (or any page) already having been visited, so added a drupalGet() to those.

Otherwise everything works, so that proves this is completely unnecessary in BrowserTestBase, I wonder how many millions of requests we've done over the years.

I think my last question is - do we want to detect xdebug.start_with_request = trigger and only do the request when that's set? But we could maybe tackle that in a follow-up.

I think my last question is - do we want to detect xdebug.start_with_request = trigger and only do the request when that's set? But we could maybe tackle that in a follow-up.

Definitely followup material as I think there are other conditions that might make this hard to detect, like if it is "default" instead of "trigger" then it depends on what xdebug.mode is set to.

Yeah that makes sense, I'm also not entirely sure that's the only thing we need it for, there could be something else copying the xdebug pattern somewhere.

Needs a rebase.

Job runtimes on the latest MR run look extremely good here. When profiling this is something like 2-300ms per test. Even though the what will become the first request with these removed will do some of the cache building that the front page visit would have done, it it's still probably 1-200ms saving per test method.

The slowest job was 2m45s, but 45s of that was runner setup time, these used to be well over 3m30s each.. #3614817: Reduce functional test job CPU and concurrency will bump those times up again slightly but we have at least a couple more issues left on #3615979: [meta] Reduce container rebuilds in the installer and functional/kernel tests to reduce times further, so we're going to end up with both marginally faster test times and a lot less CPU usage overall.

Updated the title and issue summary a bit to match the current approach.

Just coming here after noticing #3608414: Skip the pre-flight front-page HTTP request in BrowserKitTrait::setupMinkSession() when there is no XDEBUG_SESSION cookie is causing issues when using DrupalSelenium2Driver, @mstrelan pointed me here as it's similar.

Just wanted to point out we definitely do always need the visit in WebDriverTestBase otherwise anything calling executeScript early will fail with something like
Failed to read the 'sessionStorage' property from 'Window': Storage is disabled inside 'data:' URLs.

In our case, we are testing that javascript errors correctly fail our JS tests, we have a PreCondition that clears out js_testing_log_test.errors in sessionStorage, but when using the DrupalSelenium2Driver, executeScript calls out to processJavascriptDeprecations which tries to evaluate js_testing_log_test.warnings and fails because there is no active document

@longwave yeah we have something like that in our custom code:
$this->getSession()->executeScript("try { sessionStorage.removeItem('js_testing_log_test.errors'); } catch (e) {}");

I'm not sure where we'd document the issue though

The good news is this patch allowed us to remove auto retries from our FunctionalJavascript test job :)

The try/catch sounds good. In the change record sounds fine for documentation.

Should we add a separate CR to explain that tests no longer visit a frontpage by default? Or fold it into the deprecation CR?

Unfortunately it looks like we can't move the visit in WebDriverTestBase::initMink inside a conditional based on the existence of $cookies because drupalGet calls out to prepareRequest which sets a cookie so without the visit you get Behat\Mink\Exception\DriverException: The driver is not started.

I think we can use one CR for this, it's all pretty related.

I'm a bit confused where things sit between #25/#27 and #32, does #32 mean throwing the exception won't work?