Problem/Motivation

Simpletest has a hardcode minimum memory of 320mb! Due to #2422019: Don't use reflection for parsing test annotations this is not necessary.

Proposed resolution

  • Reduce recommended memory for simpletest to 192M.
  • Fix run-tests.sh to run PHPUnit tests in forked processes. This means that the parent test-runner does not use lots of memory.
  • Fix abstract MTimeProtectedFileStorageBase to allow concrete implementations to run in parallel

Following these changes the only test that requires more the 128M is Drupal\migrate_drupal\Tests\d6\MigrateDrupal6Test . it needs around 130M. This is not surprising since this collects all the d6 migration tests and runs them together.

Remaining tasks

Review
Commit

User interface changes

None

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because nothing is currently borken.
Unfrozen changes Unfrozen because it only changes tests and code that runs tests.
Disruption None
CommentFileSizeAuthor
#27 2422745.26.patch3.66 KBalexpott
#27 24-26-interdiff.txt797 bytesalexpott
#24 2422745.24.patch3.66 KBalexpott
#10 2422745.10.patch4.1 KBalexpott
#8 2422745.7.patch1.1 KBalexpott
#4 2422745.4.patch4.1 KBalexpott
#3 2422745.3.patch1.05 KBalexpott
#1 2422745.1.patch1.1 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Active » Needs review
FileSize
2422745.1.patch1.1 KB

Let's see what breaks at 128mb

Status: Needs review » Needs work

The last submitted patch, 1: 2422745.1.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
FileSize
2422745.3.patch1.05 KB

So only two tests fail at 128M!!!!
Drupal\migrate_drupal\Tests\d6\MigrateDrupal6Test - not that surprising given that it is a meta test.
Drupal\Tests\Core\Entity\EntityManagerTest - a PHPUnit test!

Increasing to 192M to see if that is enough.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
2422745.4.patch4.1 KB

I think EntityManagerTest is only failing in #1 due to the way we run PHPUnit tests in run-tests.sh.

The attached patch speeds up running the PHPUnit tests by 30 seconds with a concurrency of 8. Atm a PHPUnit test is a blocking :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

In MTimeProtectedFileStorageBase we can't use vfsStream because tempnam() is not supported https://github.com/mikey179/vfsStream/wiki/Known-Issues. Imo this is a bit of a dodgy unit test because it is changing stuff in the temporary file system.

Status: Needs review » Needs work

The last submitted patch, 4: 2422745.4.patch, failed testing.

The last submitted patch, 3: 2422745.3.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
FileSize
2422745.7.patch1.1 KB

Proper 192M patch.

Status: Needs review » Needs work

The last submitted patch, 8: 2422745.7.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
FileSize
2422745.10.patch4.1 KB

Ah... a proper 192M patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

We need to fix the phpunit tests to run in a forked process because running them in the test runner process takes memory.

The last submitted patch, 3: 2422745.3.patch, failed testing.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue tags: +Performance

we have a bunch of reduce memory related issues, but most (seem to be) around install. like #2289201: [Meta] Make drupal install and run within reasonable php memory limits so we can reset the memory requirements to lower levels

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Nice, I guess this means that phpunit tests run concurrent now, like other tests? Probably doesn't make much of a difference now, but if make KernelTestBaseV2 happen, it will..

The last submitted patch, 3: 2422745.3.patch, failed testing.

isntall queued 8: 2422745.7.patch for re-testing.

isntall queued 4: 2422745.4.patch for re-testing.

isntall queued 3: 2422745.3.patch for re-testing.

isntall queued 1: 2422745.1.patch for re-testing.

The last submitted patch, 3: 2422745.3.patch, failed testing.

The last submitted patch, 1: 2422745.1.patch, failed testing.

The last submitted patch, 8: 2422745.7.patch, failed testing.

The last submitted patch, 4: 2422745.4.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Issue summary: View changes
FileSize
2422745.24.patch3.66 KB

Patch to review/commit.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Issue summary: View changes
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented 
+++ b/core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageBase.php
@@ -69,7 +70,7 @@ public function testSecurity() {
     $php->save($name, '<?php');
-    $expected_root_directory = sys_get_temp_dir() . '/php/test';
+    $expected_root_directory =  $this->directory . '//test';
     if (substr($name, -4) === '.php') {

Are you sure that //test is correct?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
24-26-interdiff.txt797 bytes
2422745.26.patch3.66 KB

Doh!

Berdir’s picture

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

Thanks, I think this is good to go then.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed ad2a66d on 8.0.x 
    Issue #2422745 by alexpott: Reduce simpletest memory limit

Status: Fixed » Closed (fixed)

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