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

Comments

damiankloip’s picture

damiankloip commented

When I can actually upload the patch, I will.

damiankloip’s picture

damiankloip commented
Status: Active » Needs review
StatusFileSize
new 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 commented
Issue summary: View changes

Updated issue summary.

dawehner’s picture

dawehner
Primary language German
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
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 commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2104123-5.txt13.27 KB
new 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 commented
Status: Needs work » Needs review
StatusFileSize
new 2104123-7.patch24.22 KB

Forgot to rebase branch.

dawehner’s picture

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

Great!!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
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) commented
Issue summary: View changes

Updated issue summary.