Don't use reflection for parsing test annotations

It is going to be problematic to make doctrine's annotation parser play nice with PHPUnit type annotations since they expect an open parenthesis.
Compare the annotation in the node entity

 * @ContentEntityType(
 *   id = "node",

with a simpletest annoation

 * @group system

Had a look to see how easy it would be to make Doctrine's annotation parser parse a simpletest / phpunit annotation. It is not simple. The lexer is not built to work this way.

Another idea... I've confirmed that the all the test classes are listed when you use the --list option on run-tests.sh. Because we can't determine if something is a PHPUnit test then all the groups are different - which is why I've standardised on using ucfirst().

Before

After

Now lets use the same logic everywhere.

Now with tests.

I think the regex to get the first summary line can be improved.

re #7 I don't think that'll work unfortunately.

    /**
     * {@inheritDoc}
     */
    public function isSubclassOf($class)
    {
        throw new ReflectionException('Method not implemented');
    }

from StaticReflectionClass.

Improved the summary line parsing and added yet more tests.

A 200mb memory reduction must qualify this as a performance critical - plus the relief to testbots will be huge and it is good for new contributors because the testing page will work without huge amounts of memory for php.

Not sure why the ucfirst() is added here? We've been discussing that in #2301481: Mark test groups as belonging to modules in UI.

Ucfirst() is added because this patch removed the php unit group making duplicate groups even more prominent.

Hm, that is an interesting change, because lot of .travis scripts out there rely on phpunit tests not being in the module (they run phpunit test and then the group tests separately), and they also hardcode the group name. Including my script for http://d8ms.worldempire.ch/, I quite like that I can report phunit/simpletest status separately.

Not saying that we shouldn't do that, just to be aware of the side effects that will have.

Without using something more funky for static reflection (https://github.com/sun/staticreflection whihch looks unmaintained since the original development) we can not know that it is a PHPUnit test - I tried using the use statements but things like BlockContentLocalTasksTest which extends LocalTaskIntegrationTest which extends UnitTestCase make that unrealistic.

If people want to report PHPUnit separately run the PHPunit tests using phpunit. Yep they will get re-run on when using run-tests.sh but I think that is a small price to pay for the gain here.

@Berdir that said there is a way :) - the namespace all PHPUnit tests start with Drupal/Test/ and simpletest tests cannot have that so... fixed and removed the ucfirst for the other issue.

Thanks, I'm not sure if it is better to use ucfirst() or not, but I think it is good to discuss that separately and not enforce it as part of a critical issue.

Did not review the patch closely, but +1 to RTBC from me.

Looks good to me as well, found a few nitpicks though :/

1. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -144,24 +152,16 @@ public function getTestClasses($extension = NULL) {
   +        // skip it.  Most likely it is an abstract class, trait or test fixture.
   

   Extra space here.

2. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -290,76 +293,53 @@ public static function scanDirectory($namespace_prefix, $path) {
       * @throws \LogicException
       *   If the class does not have a PHPDoc summary line or @coversDefaultClass
       *   annotation.
   

   This should be MissingSummaryLineException now.

3. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -368,32 +348,26 @@ public static function getTestInfo(\ReflectionClass $class) {
   -   *   The parsed phpDoc summary line.
   +   *   The parsed phpDoc summary line. Empty
   

   Cliffhanger :P

Thanks @amateescu - fixed all of that.

Committed/pushed to 8.0.x, thanks!

+++ b/core/modules/simpletest/src/Form/SimpletestResultsForm.php
@@ -153,7 +153,7 @@ public function buildForm(array $form, FormStateInterface $form_state, $test_id
diff --git a/core/modules/simpletest/src/TestDiscovery.php b/core/modules/simpletest/src/TestDiscovery.php

+++ b/core/modules/simpletest/src/TestDiscovery.php
@@ -287,79 +290,56 @@ public static function scanDirectory($namespace_prefix, $path) {
+    // Force all PHPUnit tests into the same group.
+    if (strpos($classname, 'Drupal\\Tests\\') === 0) {
+      $info['group'] = 'PHPUnit';

Why do all PHPUnit tests grouped together? For instance a PHPUnit test for blocks should be grouped in block. Not in PHPUnit. If I make a change to the block module and I want to do a quick test to see if all tests for the block-module pass the PHPUnit block tests are not run.

That's how it worked before, and we agreed to not change the current behavior here.

Thank you @Berdir and @dawehner for pointing out to me that I should create a new issue for that. :-)

The new issue for the grouping of PHPUnit test (#2427191: Test-runner ignores @group annotations for PHPUnit-based tests in modules.).

Manuel Garcia and I lost 40+ minutes trying to figure out why #2499971: Tests are missing @coversDefaultClass annotations was happening as a result of insufficient documentation related to this issue. Cross-linking #2325985: No documentation about @group @coversDefaultClass @covers.

Ok, did find a change record for this after all: https://www.drupal.org/node/2325985. Added this issue to related issues there.

Spun off #2500031: Killing the entire Testing list page if any test anywhere is missing a summary is not nice; add robustness to make this a bit more robust.