Break BrowserTestBase & children dependency on Simpletest, deprecate stub BC traits

Well, we cannot remove simpletest until Drupal 9. I think this is for sure. One thing though we have to ensure is that BrowserTestBase is better, in every sense.
If its better people will switch, as it gives them advantages.

Regarding the dependency breaking. I think this in general a good idea, but we ideally do that in small portions, as we do already, like introducing new traits, etc.
Do you think we should mark this particular issue more as a plan?

We can deprecate the traits in simpletest module, copy them to core tests somewhere and then empty the originals and make them extend the moved ones. Right?

Couldn't even the simpletest base just extend the existing traits?

Changed title for gender neutral language. I thought subclasses are daughters? :-P

Updated issue summary with some stuff. Feel free to disagree. :-)

I also want to point out that it works both ways: Simpletest sometimes needs classes in core/tests but doesn't have them loaded. See #2745123: Simpletest module crashes on cleanup, uninstall

+1 on the solution in #4. Move the traits and have simpletest extend them with empty deprecated traits.

See also: #2672762: Move core/modules/simpletest/src/RouteProvider.php to the Drupal\Tests namespace/folder

Here's a tentative first step to untangle the traits. Presented to see what the testbot finds that I didn't.

This patch moves Drupal\simpletest\AssertHelperTrait to Drupal\Tests\AssertHelperTrait, and leaves a deprecated stub trait in simpletest.

We should follow a deprecate/remove usage pattern. This issue should deprecate the traits and then other issues can convert usages, either all in one go or per-trait.

@Mile23: Looks like you forgot to add the new file for your patch?

Patch -> depends on -> coffee.

Here's some wider scope: #2863044: [plan] Remove simpletest dependencies from PHPUnit-based tests

+++ b/core/modules/simpletest/src/AssertHelperTrait.php
@@ -2,34 +2,16 @@
 namespace Drupal\simpletest;
 
-use Drupal\Component\Render\MarkupInterface;

we need @trigger_error() here now for deprecation notices.

Should we also copy AssertHelperTraitTest? We probably just have to extend the class and change the namespace?

+++ b/core/tests/Drupal/Tests/AssertHelperTrait.php
@@ -33,3 +33,4 @@ protected static function castSafeStrings($value) {
 }
+

unrelated change, should be removed.

This patch deals with the issues in #17.

This gives us a deprecation for AssertHelperTrait but not the others.

It also leaves these usages which we'll have to deal with in a follow-up:

Found 7 matches of Drupal\simpletest\AssertHelperTrait in 7 files.	
BookUninstallValidatorTest.php	
use Drupal\simpletest\AssertHelperTrait;      [position 5:5]	
FieldUninstallValidatorTest.php	
use Drupal\simpletest\AssertHelperTrait;      [position 5:5]	
FilterUninstallValidatorTest.php	
use Drupal\simpletest\AssertHelperTrait;      [position 5:5]	
ForumUninstallValidatorTest.php	
use Drupal\simpletest\AssertHelperTrait;      [position 5:5]	
KernelTestBase.php	
use Drupal\simpletest\AssertHelperTrait;      [position 21:5]	
BrowserTestBase.php	
use Drupal\simpletest\AssertHelperTrait;      [position 25:5]	
RequiredModuleUninstallValidatorTest.php	
use Drupal\simpletest\AssertHelperTrait;      [position 5:5]	

Hmmm... So RunCronTrait is in Drupal\Tests\Traits\Core\RunCronTrait which I think is a good idea.

Or at least to normalize on Drupal\Tests\Traits as a place to put all these traits.

Moving those is a good step! Thank you for creating the patch!

Moved the rest.

+++ b/core/modules/simpletest/src/BlockCreationTrait.php
@@ -2,66 +2,20 @@
-use Drupal\block\Entity\Block;
+use Drupal\Tests\BlockCreationTrait as BaseBlockCreationTrait;
+

+++ b/core/modules/simpletest/src/ContentTypeCreationTrait.php
@@ -2,53 +2,20 @@
+use Drupal\Tests\ContentTypeCreationTrait as BaseContentTypeCreationTrait;

I am not convinced that node related stuff belongs into the generic testing framework. what are your thoughts about that?

In the case of BlockCreationTrait, Block is an entity in Core\ and not dependent on a module such as node.

If we want these traits to be more explicitly dependent on modules, such as ContentTypeCreationTrait needing node, then we should put the trait in core/modules/node/tests/src/Traits, because that's the only place they can autoload from. Then only BrowserTestBase (and the javascript subclass) can use it, and KernelTestBase if node module is enabled.

Also, a bunch of tests re-declare that they use some of these traits, presumably so they can call for instance ContentTypeCreationTrait::createContentType() instead of the aliased method at drupalCreateContentType(). See Drupal\Tests\big_pipe\FunctionalJavascript\BigPipeRegressionTest for an example.

If we want these traits to be more explicitly dependent on modules, such as ContentTypeCreationTrait needing node, then we should put the trait in core/modules/node/tests/src/Traits, because that's the only place they can autoload from. Then only BrowserTestBase (and the javascript subclass) can use it, and KernelTestBase if node module is enabled.

+1 to that.

Also, a bunch of tests re-declare that they use some of these traits, presumably so they can call for instance ContentTypeCreationTrait::createContentType() instead of the aliased method at drupalCreateContentType(). See Drupal\Tests\big_pipe\FunctionalJavascript\BigPipeRegressionTest for an example.

I think we added the ablity to use drupalCreateContentType() to keep the BC. We should deprecate the use of drupalCreateContentType() in KTB, BTB and JTB. We should also create an issue to replace the drupalCreateContentType()with createContentType().

Traits moved to:

Drupal\Tests\block\Traits\BlockCreationTrait
Drupal\Tests\node\Traits\ContentTypeCreationTrait
Drupal\Tests\node\Traits\NodeCreationTrait
Drupal\Tests\user\Traits\UserCreationTrait

Updated use statements in BrowserTestBase.

Drupal\Tests\block\Traits\BlockCreationTrait
Drupal\Tests\node\Traits\ContentTypeCreationTrait
Drupal\Tests\node\Traits\NodeCreationTrait
Drupal\Tests\user\Traits\UserCreationTrait

is fine I guess? It just won't do anything if the namespaces aren't available, right?

The Drupal\Tests\[module]\Traits\ namespace is the only thing that's always autoloadable for all the different testsuites, after #2499239: Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests

This doesn't remove usages, and deprecates the Drupal\simpletest\ namespaced traits that are in use by BTB.

This leads to lovely deprecation notices which do mention the trait, but fail to mention the class which uses them. Unfortunately we can't use __TRAIT__ or __CLASS__ to deliver such a message because the trait hasn't been named yet when execution gets to @trigger_error() and the class is still being parsed at that point, too.

However, we can still say for example BaseAssertHelperTrait::class in the error message, which might help us if we move things around again. :-) This resolves to the actual trait name (without Base), so all is well.

The fail in #28 shows us that a) the symfony phpunit bridge works... and b) that we won't be removing usages in a follow-up. :-)

This patch changes all usages to the new traits from the deprecated ones. It will still fail because this doesn't make the changes that will be handled in #2876145: ConfigTranslationViewListUiTest is a WTB test in the Functional namespace

Update... OK, maybe it won't fail. Still, there are similar changes in #2876145: ConfigTranslationViewListUiTest is a WTB test in the Functional namespace

Retesting #28 now that #2878248: DrupalStandardsListener improper handling of @trigger_error() deprecation is in, because we want to avoid removing usages in the deprecation issue.

Nice work @Mile23!

Assuming you mean #30. I guess we can bypass the normal deprecation process of deprecating before changing usage. It seems like that's how it will have to work now with @trigger_error().

Patch looks fine I think, but do we want a change record for this one?

Added change record: https://www.drupal.org/node/2884454

Needed a re-roll, too.

Sweeping up the deprecations after #2870453: Convert web tests to browser tests for node module and maybe others.

So here's something to keep in mind:

When we commit this, we haven't simply marked all those traits as deprecated. All tests which access the deprecated traits will always fail, which is OK if that's the desired outcome. I'm not sure how many patches this will effect, but a maintainer can answer whether rerolls are acceptable.

While kernel, functional, and functionalJavscript tests won't fail when they should for other types of deprecation (due to the way isolated tests work... see #2870194-31: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code) these trait deprecations are triggered during discovery, during include. That's why that last set of failures were splashed by the autoloader, like this:

Drupal\simpletest\NodeCreationTrait is deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.0. Use Drupal\Tests\node\Traits\NodeCreationTrait instead: 1x
    1x in ClassLoader::loadClass from Composer\Autoload

Also, this means that @group legacy won't exclude tests from fails for test which touch these traits in any way. In fact, the only way to turn off such deprecation fails is to remove the listener from phpunit.xml.dist.

But, that just means we're getting rid of simpletest sooner, rather than later.

The patch. :-)

Let's split this issue up so it's not a nightmare to review, and so we don't have the consistency problems outlined in #39.

We'll deprecate the old traits, but we won't @trigger_error().

A follow-up will add the @trigger_error() and make the tests pass.

Thanks, @Mile23 for the patch.
We have a separate issue for adding @trigger_error to the deprecated classes. #2886547: Add trigger_error to deprecated TestBase base classes/traits that have none, reference CR We could specify about traits in that issue.

#43,
I like the idea of having BTB its own traits and deprecating WTB traits.However, Asserts method could not be the same b/c accessing the value in WTB is different from BTB NodeElement. I don't have the use case for now but anyhow we're copying the files so we could overwrite it later.

+++ b/core/tests/Drupal/Tests/AssertHelperTraitTest.php
@@ -1,19 +1,13 @@
+ * @coversDefaultClass \Drupal\Tests\AssertHelperTrait

This change should pass the cache tests pass #2863318: Cache: Convert system functional tests to phpunit

We have a separate issue for adding @trigger_error to the deprecated classes.

I feel pretty certain that we'd want to add @trigger_error() for these different testing systems at different times. Also, it's not entirely clear whether @trigger_error() really works for a lot of our use-cases. See the comment I left over there: #2886547-22: Add trigger_error to deprecated TestBase base classes/traits that have none, reference CR

I like the idea of having BTB its own traits and deprecating WTB traits.

That's not what's happening here. We are isolating BTB from any dependency on the simpletest module or any simpletest namespace. WTB traits are only deprecated in that they now stub out to other traits in a different namespace.

However, Asserts method could not be the same b/c accessing the value in WTB is different from BTB NodeElement. I don't have the use case for now but anyhow we're copying the files so we could overwrite it later.

Yes, def figure out a follow-up for that.

Also, @coversDefaultClass only helps in a coverage report, and shouldn't change the behavior of the test.

This looks ready to go, one little question,

+++ b/core/modules/simpletest/src/AssertHelperTrait.php
@@ -2,34 +2,16 @@
+ * @deprecated in Drupal 8.4.x. Will be removed before Drupal 9.0.0. Use
+ *   Drupal\Tests\AssertHelperTrait instead.

Since we have a CR, should we not add a 'See https://www.drupal.org/node/2884454.' to all the deprecation messages?

Good call. Updated with @see linking to the CR.

I'm calling this the follow-up: #2886547: Add trigger_error to deprecated TestBase base classes/traits that have none, reference CR See especially comment #22: #2886547-22: Add trigger_error to deprecated TestBase base classes/traits that have none, reference CR

This looks ready to me, CR looks up to date, IS looks up to date.

Would have been nice to get this in 8.4.x, but it seems disruptive enough to need to go in 8.5.x.

If anyone has a different opinion please speak up. :-)

Re-testing against 8.5.x.

Personally I'm cool with it to be 8.5.x only .. for me its all about progress, but not necessarily in which version.

Committed 5f77cf7 and pushed to 8.5.x

Published the change record.

Have asked other committers thoughts on backport.

discussed with @catch and agreed to backport

cherry picked as 71e5829 and pushed to 8.4.x

updated change record

Yeah it seems like a good idea for this to be backported. Correcting the branch to reflect the backport.

This might also be worth mentioning in a "testing improvements" section in the release notes?

You know, it's funny that a room full of smart people don't realize they need this follow up: #2907169: Break KernelTestBase dependency on Simpletest, deprecate stub BC traits

(That's self-deprecating humor in case you didn't realize.)

Doh. :)

Clean up here: #2922371: Numerous deprecation messages for test traits do not reference the correct replacement