Add trigger_error to deprecated TestBase base classes/traits that have none, reference CR

Mh, I think its not worth the risk. Contrib might use it.

@naveenvalecha thanks for all the work you've been doing on the PHPUnit Initiative issues. Really appreciated.

The contents of automated tests provided by core modules are never considered part of the API. While the testing framework itself may be treated as an API individual test classes are always internal.

Thats the part of the BC policy @naveenvalecha linked to in the parent issue. I would argue that the base classes are part of the testing framework. And while this particular base class might not get used a lot (or at all), I don't think we would have this discussion about \Drupal\views\Tests\ViewTestBase or \Drupal\node\Tests\NodeTestBase. And I think having one clear policy about base classes (Deprecate them, leave them where they are) helps make reviewing these conversions easier.

So big +1 for leaving it in and removing them only when we remove Simpletest from core.

One little nitpick:

+++ b/core/modules/aggregator/src/Tests/AggregatorTestBase.php
@@ -2,6 +2,8 @@
+@trigger_error('\Drupal\aggregator\Tests\AggregatorTestBase is deprecated and will be removed before Drupal 9.0.0. Use \Drupal\Tests\aggregator\Functional\AggregatorTestBase. See https://www.drupal.org/node/2886547.', E_USER_DEPRECATED);

I don't think we need the 'see', git blame will take you here anyway if you are really interested.

Nah, don't care that much, I think there is a mix of trigger_error with and without 'see' in core already.

Thanks @naveenvalecha and @Lendude!

The thing with the "See..." is that it's supposed to reference a change record, not this issue. If it were just this issue, I'd agree, people can use git blame. But the change record is intended to tell someone how to convert the test. So in this case, it should be the general change record on BTB probably.

Thanks!

@xjm thanks for the feedback and @naveenvalecha thanks for the new patch.

I am positive there is a change record for WTB -> BTB because I insisted there be one as a prerequisite for the initaitive. :)

https://www.drupal.org/node/2469723 looks like the right one to me. Moving back to CNW for the change.

@catch++

+++ b/core/modules/aggregator/src/Tests/AggregatorTestBase.php
@@ -2,6 +2,8 @@
+@trigger_error('\Drupal\aggregator\Tests\AggregatorTestBase is deprecated and will be removed before Drupal 9.0.0. Use \Drupal\Tests\aggregator\Functional\AggregatorTestBase. See https://www.drupal.org/node/2469723.', E_USER_DEPRECATED);

lets use the __NAMESPACE__ pattern we used elsewhere, I kinda like that, easier to spot that we are talking about the current class.

Discussed with @naveenvalecha on IRC, seems a bit pointless to only add this 'See' to this testbase class. I think all the deprecated testbase classes would really benefit from that 'See'. So expanding the scope of this to update all deprecated test base classes and will update the conversion documentation to include this as well.

Okay, so this is somewhat problematic, and Im trying to sort out what the right way to make this work.

1. E_USER_DEPRECATED errors are thrown by phpunit, and phpunit converts all messages to exceptions. Any test that hits E_USER_DEPRECATED will therefore be a failing test. phpunit does not currently have a way to suppress this behavior from the configuration (see https://stackoverflow.com/questions/43726162/phpunit-deprecation-warning...)
2. Contrib modules that want to simultaneously work with the current stable version of core (8.3.4), as well as the supported(8.3.x), development (8.4.x), and prerelease (none now, but 8.4.x later on when 8.5.x opens) branches are therefore unable to have their tests pass on all environments because either they continue to use the deprecated function (because it doesnt exist in 8.3.4), and have their 8.4.x tests fail, or they use the new function, and cannot be used with 8.3.4/8.3.x anymore. An example of this is [2890024-12145937] (Drupal\taxonomy\Tests\TaxonomyTestTrait is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, use \Drupal\Tests\taxonomy\Functional\TaxonomyTestTrait)

So, Im not sure what the right thing to do here is. We could suppress E_USER_DEPRECATED on the testbots, but that kinda defeats the purpose it seems. We could do something to capture those Deprecation errors in run-tests.sh or simpletest. We basically need a way that alerts contrib developers that they are using deprecated testing api's, but does *not* result in a test failure for doing so, because they really do not have a choice if the alternative does not yet exist in the current stable (i.e. \Drupal\Tests\taxonomy\Functional\TaxonomyTestTrait is not in 8.3.4)

E_USER_DEPRECATED errors are thrown by phpunit, and phpunit converts all messages to exceptions. Any test that hits E_USER_DEPRECATED will therefore be a failing test. phpunit does not currently have a way to suppress this behavior from the configuration (see https://stackoverflow.com/questions/43726162/phpunit-deprecation-warning...)

Can't they use @legacy here? https://symfony.com/doc/current/components/phpunit_bridge.html#mark-test...

I tried the @legacy group tag, but it still caused phpunit to return 1...

There are a few problems here:

1) The behavior of @trigger_error() for simpletest-based tests is undefined. That is: TestBase and WebTestBase shouldn't really count on @trigger_error(), because the whole point of @trigger_error() is that symfony/phpunit-bridge knows what to do with the error. symfony/phpunit-bridge doesn't ever have an opinion about WTB, because phpunit is not used to run those tests. This suggests that we should extend symfony/phpunit-bridge to include simpletest behaviors, or at least figure out how to turn it all off for simpletest testing. I think we should change as little as possible in the simpletest ecosystem as long as things work, even if it means deprecation errors don't bubble up to the test runner. Speaking of which, run-tests.sh has no way to store or process a deprecation message. Woot.

2) Moving WTB tests to BTB still leaves us with a problem, and that is that @trigger_error() will never reach symfony/phpunit-bridge's error handler because BTB is always run in a separate process. That is: symfony/phpunit-bridge does not deal with tests which are run in a separate process. https://github.com/symfony/symfony/issues/23003 And our issue: #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code

3) If @trigger_error() happens just after the namespace statement, then any process which parses the file will trigger the error. That means it will happen during discovery. This is why marking your test with @group legacy doesn't have any effect.

This was all discovered here: #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code

With a stop-gap solution here: #2884530: Mark process-isolated test with @expectedDeprecation as risky

And obviously the upstream solution which I could never get to work because the symfony testing setup is less straightforward than ours. (for reals.) https://github.com/symfony/symfony/issues/23003

When we figure out how to do this, it should include deprecated simpletest traits: #2803621: Break BrowserTestBase & children dependency on Simpletest, deprecate stub BC traits

Moving WTB tests to BTB still leaves us with a problem, and that is that @trigger_error() will never reach symfony/phpunit-bridge's error handler because BTB is always run in a separate process. That is: symfony/phpunit-bridge does not deal with tests which are run in a separate process.

This is no longer true: #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code

issue fixed for 8.6.x

Thanks to #2973127: Exclude *TestBase.php, *Trait.php and *Interface.php files from test discovery reflection and #2949363: Impossible to make trigger_error in some files without test fails we're now able to trigger deprecation errors for files that end in *TestBase.php with impunity.

That leads us to some of the ambiguity of this issue: Does it reference all *TestBase.php files? Or only TestBase the class?

I'm guessing the latter, since the example in #16 is a simpletest-based test.

Totally down with adding @trigger_error() to all simpletest-based base classes that are already deprecated. This patch does @trigger_error() for all simpletest-based tests that end in *TestBase.php. :-)

This patch will need updated deprecation messages (including links to change records), but they all @trigger_error() so that the testbot tells us what will break as a consequence.

Regarding change records (@xjm #12):

Using AggregatorTestBase as an example, it was deprecated in #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits which doesn't have a CR. Its parent #2807237: PHPUnit initiative also doesn't have a CR.

So marking #2807237: PHPUnit initiative with a needs change record tag.

Totally down with adding @trigger_error() to all simpletest-based base classes that are already deprecated. This patch does @trigger_error() for all simpletest-based tests that end in *TestBase.php. :-)

That sounds like a good place to start. Not setting this to rtbc because we don't have the change record yet. Otherwise this is looking like a great start.

Widening scope a bit, because we have a number of TB base classes and traits that are deprecated but don't trigger errors.

This patch adds @trigger_error() to all deprecated base classes and traits which don't already have them, within Drupal\module\Tests and Drupal\simpletest namespaces.

It does NOT fix the generic deprecation messages from #29 per #31. That's the next step, after we discover whether adding so many @trigger_error() surfaces more tech debt than we really want.

Reverts the errors triggered by AssertPageCacheContextsAndTagsTrait and AssertContentTrait because they're used by WebTestBase, and the replacements don't seem to be drop-in replacements. And since we have PHPUnit-based tests of WebTestBase, those fail based on errors.

Also, we should move AssertContentTraitTest out of simpletest, since we're now testing Drupal\KernelTests\AssertContentTrait.

1. +++ b/core/modules/aggregator/src/Tests/AggregatorTestBase.php
   @@ -2,6 +2,8 @@
    
   +@trigger_error(__FILE__ . ' is deprecated. Use a PHPUnit functional test instead.', E_USER_DEPRECATED);
   

   Should we link to https://www.drupal.org/project/drupal/issues/2735005 given it describes how to convert these tests?

2. +++ b/core/modules/config/src/Tests/SchemaCheckTestTrait.php
   @@ -2,10 +2,13 @@
    
   +@trigger_error(__NAMESPACE__ . '\SchemaCheckTestTrait is deprecated as of 8.3.x, will be removed in before Drupal 9.0.0. Use \Drupal\Tests\SchemaCheckTestTrait instead.', E_USER_DEPRECATED);
   +
   

   Should we have a change record for that?

#36.1:

+@trigger_error(__FILE__ . ' is deprecated. Use a PHPUnit functional test instead.', E_USER_DEPRECATED);
Yes, definitely all of these need to be researched and turned into proper messages.

#36.2: Many do not have change records. I concentrated on adding @trigger_error() to reveal tech debt.

Also, did you know that there's no change record for the PHPUnit initiative?

Added a CR for #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits because it didn't have one: https://www.drupal.org/node/2999939

Added this issue to that CR.

Put all that in the deprecation errors and the docblocks.

This looks great.

Went through all the files and these were the only mismatches I could find:

1. index f42248975a..7570c0493f 100644
   --- a/core/modules/aggregator/src/Tests/AggregatorTestBase.php
   
   --- a/core/modules/aggregator/src/Tests/AggregatorTestBase.php
   +++ b/core/modules/aggregator/src/Tests/AggregatorTestBase.php
   
   @@ -2,6 +2,8 @@
   +@trigger_error(__NAMESPACE__ . '\ContentTranslationTestBase is deprecated for removal before Drupal 9.0.0. Use \Drupal\Tests\aggregator\Functional\AggregatorTestBase instead. See https://www.drupal.org/node/2999939', E_USER_DEPRECATED);
   

   wrong class name in the trigger_error

2. +++ b/core/modules/block/tests/src/Kernel/BlockRebuildTest.php
   index a60ab91d4b..2db2cb6ff3 100644
   --- a/core/modules/block_content/src/Tests/BlockContentTestBase.php
   
   --- a/core/modules/block_content/src/Tests/BlockContentTestBase.php
   +++ b/core/modules/block_content/src/Tests/BlockContentTestBase.php
   
   +++ b/core/modules/block_content/src/Tests/BlockContentTestBase.php
   @@ -2,6 +2,8 @@
   +@trigger_error(__NAMESPACE__ . '\ContentTranslationTestBase is deprecated for removal before Drupal 9.0.0. Use Drupal\Tests\block_content\Functional\BlockContentTestBase instead. See https://www.drupal.org/node/2999939', E_USER_DEPRECATED);
   

   Wrong class name in the trigger_error

Rerolled and fixed points from #39

The review points by @Lendude were fixed.

Committed 8c6c340 and pushed to 8.7.x. Thanks!

There are various format problems with the deprecation messages introduced here. First the bigger problem is where we say we'll remove something in 8.x.y. Opened #3033317: AccountProxy initialAccountId and ViewKernelTestBase will only be removed in Drupal 9.0.0, fix deprecation messages for that.