CommentFileSizeAuthor
#19 2254203-17.patch30.64 KBneclimdul
#18 2254203-17.interdiff.txt1.2 KBneclimdul
#13 2254203-13-interdiff.txt1.11 KBneclimdul
#13 2254203-13.patch35.42 KBneclimdul
#11 2254203-11-interdiff.txt4.64 KBneclimdul
#11 2254203-11.patch29.67 KBneclimdul
#6 module.apitest.6.patch26.16 KBsun
#5 interdiff.txt5.67 KBsun
#5 module.apitest.5.patch26.59 KBsun
#4 interdiff.txt985 bytessun
#4 module.apitest.4.patch24.64 KBsun
#2 interdiff.txt22.77 KBsun
#2 module.apitest.2.patch24.54 KBsun
testperf.system-ModuleApiTest.patch6.66 KBsun

Comments

Status: Needs review » Needs work

The last submitted patch, testperf.system-ModuleApiTest.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new module.apitest.2.patch24.54 KB
new interdiff.txt22.77 KB

Chopping off 7 minutes from the total core test suite time...

  1. Added moduleHandler() method.
  2. Fixed stupidly insane test dependency chain.

Status: Needs review » Needs work

The last submitted patch, 2: module.apitest.2.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new module.apitest.4.patch24.64 KB
new interdiff.txt985 bytes

Fixed DependencyTest; confirmation message sorts dependencies alphabetically.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new module.apitest.5.patch26.59 KB
new interdiff.txt5.67 KB

ModuleApiTest is actually ModuleHandlerTest.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new module.apitest.6.patch26.16 KB

Reverted run-tests.sh tweak.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Assigned: Unassigned » sun
Priority: Normal » Major

10% of the total core test suite time is at least major in my book.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

To be fair, it doesn't seem to be more than 30s faster in total on #1733 (27m30 instead of ~28m) due to the parallel processing. Still nice, especially when you have to run this ... ugly thing locally to fix test failures ;)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

The argument of testbots using concurrency came up in some of the test perf issues in the meantime... That's a flawed way to look at it, because:

  1. All of these tests do add up. — Fixing ~15 tests to no longer use Standard proflie *will* significantly shorten the total core test suite time.
  2. Many poor souls are still trying to run tests locally. — When it takes the testbot 10 minutes to run a test, that same single test will take 1+ hour on my box.
sun’s picture

sun
Primary language German
Location Karlsruhe
commented

6: module.apitest.6.patch queued for re-testing.

neclimdul’s picture

neclimdul
He/Him
commented
StatusFileSize
new 2254203-11.patch29.67 KB
new 2254203-11-interdiff.txt4.64 KB

Reroll post PSR-4 move and remove some tests covered by the new phpunit test. The test removals don't really help much, about a second, but they aren't needed. Interdiff against straight reroll.

Re: performance, Old test took 6 14sec min locally, new test took 19 sec. Big +1

Status: Needs review » Needs work

The last submitted patch, 11: 2254203-11.patch, failed testing.

neclimdul’s picture

neclimdul
He/Him
commented
StatusFileSize
new 2254203-13.patch35.42 KB
new 2254203-13-interdiff.txt1.11 KB

Misread that assert method. Didn't mean to delete it.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 13: 2254203-13.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest395323router' doesn't exist

That's caused by this removal (from second to last interdiff in #11):

+++ b/core/modules/system/src/Tests/Extension/ModuleHandlerTest.php
@@ -24,12 +24,6 @@ public static function getInfo() {
-  public function containerBuild(ContainerBuilder $container) {
-    parent::containerBuild($container);
-    $container
-      ->register('router.dumper', 'Drupal\Core\Routing\NullMatcherDumper');
-  }
neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs work » Needs review

Sorry about that. Documented why it existed. Swear I tested it but seems I was wrong. Made sure I didn't make another test while half asleep this time.

neclimdul’s picture

neclimdul
He/Him
commented
StatusFileSize
new 2254203-17.interdiff.txt1.2 KB

patch...

neclimdul’s picture

neclimdul
He/Him
commented
StatusFileSize
new 2254203-17.patch30.64 KB

I don't understand why its loosing my patch when I hit submit... one more time.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Excellent. Thanks @neclimdul!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review 
+++ /dev/null
@@ -1,245 +0,0 @@
-  /**
-   * Test \Drupal::moduleHandler()->getImplementations() caching.
-   */
-  function testModuleImplements() {
...
-  /**
-   * Test that moduleHandler()->invoke() can load a hook defined in
-   * hook_hook_info().
-   */
-  function testModuleInvoke() {
...
-  /**
-   * Test that \Drupal::moduleHandler()->invokeAll() can load a hook defined in hook_hook_info().
-   */
-  function testModuleInvokeAll() {

Where is the equivalent tests?

neclimdul’s picture

neclimdul
He/Him
commented

A whole series of tests for the individual cases and methods. I've done coverage tests of the implements/invoke methods though. Since what's covering what is a little unclear, best guess is this.

ModuleApiTest::testModuleImplements()
  ModuleHandlerTest::testImplementsHookModuleEnabled
  ModuleHandlerTest::testInvokeModuleEnabled
  ModuleHandlerTest::testGetImplementations
  ModuleHandlerTest::testCachedGetImplementations
  ModuleHandlerTest::testCachedGetImplementationsMissingMethod
  ModuleHandlerTest::testInvokeAll
  ModuleHandlerTest::testWriteCache
  ModuleHandlerTest::testGetHookInfo

ModuleApiTest::testModuleInvoke:
  ModuleHandlerTest::testInvokeModuleEnabled

ModuleApiTest::testModuleInvoke:
  ModuleHandlerTest::testModuleInvokeAll

Technically "testModuleList" is covered by "testGetModuleList" as well but that test covers things about install() and I wasn't sure if that was covered elsewhere. Namely writing the correct weights and then reading them back.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Reviewed & tested by the community

Thanks for answering my question - the coverage looks good.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 6c91ce8 and pushed to 8.x. Thanks!

  • Commit 6c91ce8 on 8.x by alexpott: 
    Issue #2254203 by sun, neclimdul: Fix test performance of Drupal\system\...

Status: Fixed » Closed (fixed)

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