Currently takes ~400ms on the test overview page, 27% of the whole page request.

We have to cache this.

Should be quite simple if you know a bit about caching, just check the cache first with $cache = Drupal::cache()->get('simpletest_phpunit') or something like that, return $cache-data if you get something, write the $test_classes result into the cache at the end.

CommentFileSizeAuthor
#9 cache_available_tests-2059313-9.patch2.71 KBStephaneQ
#9 cache_available_tests-2059313-9.interdiff.txt1.21 KBStephaneQ
#5 cache_available_tests-2059313-5.patch2.5 KBgiammi
#5 cache_available_tests-2059313-5.interdiff.txt711 bytesgiammi
#2 cache_available_tests-2059313-2.patch2.5 KBStephaneQ
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Issue tags: +Novice

Should be possible to handle for Novice core contributors that know a tiny bit about the caching API :)

StephaneQ’s picture

StephaneQ
Primary language French
Location Paris 🇫🇷
CreditAttribution: StephaneQ commented
FileSize
cache_available_tests-2059313-2.patch2.5 KB

Let's try with this patch.

StephaneQ’s picture

StephaneQ
Primary language French
Location Paris 🇫🇷
CreditAttribution: StephaneQ commented
Status: Active » Needs review
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Needs work 
+++ b/core/modules/simpletest/simpletest.moduleundefined
@@ -718,25 +718,34 @@ function simpletest_library_info() {
+    foreach ($test_suite AS $test) {

Not related, but that uppercase AS is weird, let's fix that while we touch this.

Code changes look good.

giammi’s picture

giammi CreditAttribution: giammi commented
Status: Needs work » Needs review
FileSize
cache_available_tests-2059313-5.interdiff.txt711 bytes
cache_available_tests-2059313-5.patch2.5 KB

trying

Berdir’s picture

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

Great, makes the test overview page quite a bit faster!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/simpletest/simpletest.moduleundefined
@@ -718,25 +718,34 @@ function simpletest_library_info() {
+  // Try to load the class names array from cache.
+  if($cache = \Drupal::cache()->get('simpletest_phpunit')) {
+    $test_classes = $cache->data;

This doesn't match core code style, see http://drupal.org/coding-standards

Also wondering whether this is desirable - what if I add a new PHPUnit test, do I need to flush caches then?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Ouch.

That you have to clear caches if you add a new tests is already the case for Simpletest. However, we probably need to add a specific cache delete to the clear environment button at the bottom.

StephaneQ’s picture

StephaneQ
Primary language French
Location Paris 🇫🇷
CreditAttribution: StephaneQ commented
Status: Needs work » Needs review
FileSize
cache_available_tests-2059313-9.interdiff.txt1.21 KB
cache_available_tests-2059313-9.patch2.71 KB

Fixed the coding style errors and added the cache delete

Berdir’s picture

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

Ok, manually verified that the clean environment button works, a newly added unit tests doesn't show up immediately (again, only in the UI, directly using phpunit works just fine and the UI for phpunit tests is broken anway) but they do after you click in Clean environment, same as with Simpletest tests.

catch’s picture

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

Committed/pushed to 8.x, thanks!

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented

It seems a little crazy to me to cache this, but hopefully it just encourages people to use use phpunit on the cli instead of simpletest as the test runner.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

The UI integration is already pretty much broken, you can't re-test something for example and there are no useful results. That's the bigger incentive to use the CLI runner.

I'd almost go as far as to suggest to remove the UI integration completely, replace it with a link to @dawehner's tutorial on how to integrate it with PhpStorm. (The last part is a joke, the first not ;))

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