Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Follow-up to #2232861: Create BrowserTestBase for web-testing on top of Mink
BrowserTestBase::prepareRequestForGenerator says the following:
protected function prepareRequestForGenerator($clean_urls = TRUE, $override_server_vars = array()) {
$request = Request::createFromGlobals();
$server = $request->server->all();
if (basename($server['SCRIPT_FILENAME']) != basename($server['SCRIPT_NAME'])) {
// We need this for when the test is executed by run-tests.sh.
// @todo Remove this once run-tests.sh has been converted to use a Request
// object.
$cwd = getcwd();
$server['SCRIPT_FILENAME'] = $cwd . '/' . basename($server['SCRIPT_NAME']);
$base_path = rtrim($server['REQUEST_URI'], '/');
}
else {
$base_path = $request->getBasePath();
}
run-tests.sh currently uses Request and has its own special kernel anyway, so it's time to do this.
Proposed resolution
Work on the @todo.
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #24 | reroll_diff_2698563_9-23.txt | 1.18 KB | ankithashetty |
| #23 | interdiff_2698563_9-23.txt | 0 bytes | ankithashetty |
| #23 | 2698563-23.patch | 1.98 KB | ankithashetty |
| |||
| #9 | 2698563_9.patch | 1.98 KB | Mile23 |
| #6 | 2698563_6.patch | 12.18 KB | Mile23 |
Comments
Comment #2
Mile23Somewhat naive first stab.
run-tests.sh has been massaged quite a bit and has much better isolation now, but I'm not sure what's up with the mink implementations. I believe we don't have any tests implementing BrowserTestBase in core, so someone from Mink-land should give a shout.
Comment #3
larowlanBrowserTestBaseTest is in core
Comment #4
Mile23Right. :-) But that's a test of BrowserTestBase, not anything else.
Comment #5
dawehnerWhen we update this piece of code we should do it in parallel to
\Drupal\simpletest\WebTestBase::prepareRequestForGeneratorwhich still has the same code. When we update WebTestBase as well, we will be able to catch way more potential problems.Never believe, try to prove your points :) The only one in core is
\Drupal\Tests\simpletest\Functional\BrowserTestBaseTestat this moment in time.Comment #6
Mile23Just my way of acknowledging that I don't know much about the status of mink testing in core.
OK, so in the name of DRY I hereby add a new trait called
RebuildContainerTrait.This trait modifies the
prepareRequestForGenerator()method as in the patch in #2 so you can look at a diff there. It also putsrebuildContainer()into the trait, since it's the same inBrowserTestBaseandWebTestBase.The trait is used by
BrowserTestBaseandWebTestbase.The methods in this trait should be marked as deprecated, since they are really only used as workarounds.
rebuildContainer()exists because we haven't finished #2021959: Refactor module handling responsibilities out of DrupalKernel andprepareRequestForGenerator()is full of side-effects which should be refactored elsewhere (and only allows GET requests). But that might be out of scope here, so the patch doesn't reflect this.Also, there are other traits such as
AssertContentTraitwhich use\Drupalto access the container, presumably because the trait would be using a property it didn't declare. These should be fixed so they use$this->container, along with another trait which declares$container.RebuildContainerTraitjust uses the properties without converting to\Drupal, because that's the wrong way to go. Adding a trait for$containeris non-trivial and might be out of scope here, though.Comment #9
Mile23#2796105: Move similar methods in BrowserTestBase / WebTestBase to a trait; untangle installDrupal() gives us
Drupal\Core\Test\FunctionalTestSetupTrait, so there's that part sorted.We still have the @todo.
I found testing locally that just removing the code that adjusts the server globals for
run-tests.shworks. Experimentally trying that with the testbot.Comment #10
Mile23Even though it's a task, this really should be 8.3.x since it reduces the complexity of the testing system.
Comment #11
Mile23Comment #21
catch#9 looks good, kicked off a test run against 10.1.x
Comment #22
smustgrave CreditAttribution: smustgrave at Mobomo commentedTook a look but appears #9 has some CI errors for 10.1.x that will need to be addressed
Comment #23
ankithashettyjust rerolled the patch in #9, thanks!
Comment #24
ankithashettyUploading the right diff file.
Comment #25
mondrakeComment #26
smustgrave CreditAttribution: smustgrave at Mobomo commentedThank you @ankithashetty
Comment #27
murilohp CreditAttribution: murilohp at CI&T commentedJust a nitpick here, do we need this empty line here?
Comment #29
catchI think the extra line is OK, if we don't like it, can remove next time this file is touched.
Committed/pushed to 10.1.x, thanks!
Comment #31
Mile23