Impossible to make trigger_error in some files without test fails

This worked for me locally, let's see if testbot agrees.

Sorry, the interdiff is correct, but the patch is messed up, let me try again.

Does the patch fail due to coding standard error ?

@zymbian no

I realise this is a work in progress but some observations

1. +++ b/core/modules/simpletest/src/TestDiscovery.php
   --- /dev/null
   +++ b/core/modules/system/src/Tests/Entity/DoNotUseMeDeprecatedTestTrait.php
   
   +++ b/core/modules/system/src/Tests/Entity/DoNotUseMeDeprecatedTestTrait.php
   @@ -0,0 +1,14 @@
   +namespace Drupal\system\Tests\Entity;
   

   This looks like a strange place to put this.

2. +++ b/core/modules/system/src/Tests/Entity/DoNotUseMeDeprecatedTestTrait.php
   @@ -0,0 +1,14 @@
   + * @deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0.
   

   No we won't remove it, we still need to be able to deprecate traits in D9!

3. +++ b/core/modules/system/src/Tests/Entity/DoNotUseMeDeprecatedTestTrait.php
   @@ -0,0 +1,14 @@
   +trait DoNotUseMeDeprecatedTestTrait {
   

   Lets come up with more descriptive name TestTraitDeprecationTrait or something?

And we need some way to indicate that this is testing something, so it won't get removed because it's not used anywhere.

So the problem is that when we add @trigger_error() to traits (and classes) we're actually deprecating the file and not the class. So whenever it's included or required, it will trigger the error.

For tests this is a non-issue because we don't deprecate tests, we just change them. For traits (and base classes) it's a problem because if the directory scan includes, requires, or reflects on the file, it will be loaded and the deprecation error is triggered.

If we disable the error handling during TestDiscovery, then there's two things:

1) The error will be 'handled' when the file loads, and we won't see the deprecation error during test time. This means we might as well not @trigger_error() in the first place. (Strictly speaking: We *might* see the error during test time, depending on which test runner, and whether we fork another process to run it.)

2) We've added complexity to TestDiscovery as a workaround that then will lead to false positives in testing, because we want things to explode (a little) when we use a deprecated class.

So basically -1 on #21.

This problem would go away as a side effect of #2296635: Evaluate performance impact of limiting test discovery to *Test.php filename suffix so we should work on that instead.

Let's adapt our test discovery to not scan TestBase.php files. Then we can deprecate TestBase classes. We can then proceed with #2296635: Evaluate performance impact of limiting test discovery to *Test.php filename suffix

And we can do the same for *Trait.php

Let's adapt our test discovery to not scan TestBase.php files. Then we can deprecate TestBase classes. And we can do the same for *Trait.php

Assuming you mean excluding *testbase.php files (we still want to scan for simpletest tests).

So basically instead of only allowing *Test.php we should exclude *TestBase.php and *Trait.php

Turning this into a meta, so we can track the later work.

Here's a specific issue: #2973127: Exclude *TestBase.php, *Trait.php and *Interface.php files from test discovery reflection

@Mile23 yep - #2973127: Exclude *TestBase.php, *Trait.php and *Interface.php files from test discovery reflection looks good.

Ah nice detective work @vaplas. So the real question is why do we bother with the real reflection? If we can't do static reflection let's assume it's not a test? Does that work...

So #41 looks good. The fail is in a test which is found and running it triggers the deprecation - no?

The combined Voltron powers of not performing reflection if it's not a test plus filtering out based on *TestBase.php and *Trait.php will yield one full solution. Should we close #2973127: Exclude *TestBase.php, *Trait.php and *Interface.php files from test discovery reflection and do it here? I guess I misunderstood all the stuff about phasing in this change.

+++ b/core/modules/simpletest/src/TestDiscovery.php
@@ -174,7 +174,7 @@ public function getTestClasses($extension = NULL, array $types = []) {
+        $info = static::getTestInfo($classname, $parser->getDocComment() ?: 'incorrect class annotation');
       }
       catch (MissingGroupException $e) {

This really should either return an empty data structure or it should throw MissingGroupException. I vote for MissingGroupException to be consistent with current behavior, and so we avoid false positives.

OK.... Helps to read the thread a little better. :-)

The problem is that if we're calling from getTestClasses() and there's no existing annotation, getTestInfo() will perform reflection. We don't want that.

The solution is to have getTestInfo() check for NULL, because it says this:

public static function getTestInfo($classname, $doc_comment = NULL) {
    if (!$doc_comment) {

So we change !$doc_comment to $doc_comment === NULL.

Then getTestInfo() will not perform reflection because it already has a docblock that happens to be an empty string. It will throw MissingGroupException for the empty docblock, including those it finds within *TestBase and so forth.

Back in getTestClasses(), we then have to figure out whether that's from a real actual test that is only missing a @group annotation, or whether it's from an abstract type, such as *TestBase. getTestClasses() does that by looking for class names that end in *Test.

#2973127: Exclude *TestBase.php, *Trait.php and *Interface.php files from test discovery reflection will help with that, too, by giving us fewer files to begin with. In fact, that single change to getTestInfo() will solve the problem for run-tests.sh and we don't actually need to filter out *TestBase etc. However, we still need to change scanDirectory() for the phpunit test suite classes: #2973127-2: Exclude *TestBase.php, *Trait.php and *Interface.php files from test discovery reflection

Thanks for working on it. :-)

In an ideal world, we'd have a good way to test whether reflection ended up happening. But we don't, other than feeding in an empty docblock and then comparing whether the class loaded or not.

Added a test. I was wrong about how to test it... We can just compare which type of exception it throws, if we have both an empty annotation and a nonexistent class.

And since it's my idea but not actually my code I can still maybe get away with RTBC. :-)

I'm doing this prior to the tests finishing, but this only adds a a test and we had success in #48.

No longer very meta :)

Committed and pushed f22c3b0952 to 8.6.x and 7257e86610 to 8.5.x. Thanks!

Backported to 8.5.x because this is the test system and making everything work the same is less headachey.

To test this patch I used the --list option on the the run-test.sh command. Before and after applying the patch had no differences even with quite a few popular contrib modules hanging about.