Updated: Comment #N

Problem/Motivation

The Drupal\system\Tests\Menu\MenuRouterTest class currently takes over 9 minutes to run (on my local machine). There are around 25 test methods, so that is that many Drupal test installations :/

Proposed resolution

Consolidate the actual test methods and proxy the current methods inside, so they don't do full Drupal installations each time.

Remaining tasks

Review, etc..

User interface changes

None

API changes

None

CommentFileSizeAuthor
#7 2104123-7.patch24.22 KBdamiankloip
#5 2104123-5.patch24.62 KBdamiankloip
#5 interdiff-2104123-5.txt13.27 KBdamiankloip
#2 d8.MenuRouterTest-refactor.patch24.56 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

When I can actually upload the patch, I will.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Active » Needs review
FileSize
d8.MenuRouterTest-refactor.patch24.56 KB

I didn't spend too much time on time on this, but running locally this brought this test class down to ~1:30 mins.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Issue summary: View changes

Updated issue summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

This is really worth considering given the amount of time saved for the bot.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/MenuRouterTest.php
@@ -633,4 +491,201 @@ public function testMenuOnRoute() {
+  function _testExoticPath() {

This has no visiblity on there in contrast to all other hunks in this patch.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
interdiff-2104123-5.txt13.27 KB
2104123-5.patch24.62 KB

Good spot. Tim also wants the methods to be called doTest* instead, so let's change that too.

Status: Needs review » Needs work

The last submitted patch, 2104123-5.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
2104123-7.patch24.22 KB

Forgot to rebase branch.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Great!!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

IMO this is a great idea, and a pattern we may want to employ in other places where there are several related tests but it's not a huge issue for the rest results of the previous test to "leak" into subsequent test runs. It manages to still keep the code nicely separated so that each test function is distinct and legible on its own.

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Updated issue summary.