Problem/Motivation

See #2862508: Convert system functional tests to phpunit
See #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase)

Convert all tests in core/modules/system/src/Tests/Entity

This also fixes some WebTestBased tests that are already in a BrowserTestBase location but currently extend a WebTestBase . See #2977105: Incomplete functional conversion for NodeCacheTagsTest

Scope:

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#21 2946419-21.patch18.29 KBLendude
#21 interdiff-2946419-18-21.txt5.11 KBLendude
#18 interdiff-12-18.txt3.92 KBAnonymous (not verified)
#18 2946419-18.patch18.19 KBAnonymous (not verified)
#12 interdiff-10-12.txt1.34 KBAnonymous (not verified)
#12 2946419-12.patch19.4 KBAnonymous (not verified)
#10 2946419-10.patch20.17 KBAnonymous (not verified)
#9 interdiff-7-9.txt1.33 KBAnonymous (not verified)
#9 2946419-9.patch35.68 KBAnonymous (not verified)
#9 interdiff-7-9-test-only.txt1.49 KBAnonymous (not verified)
#9 2946419-9-test-only.patch34.45 KBAnonymous (not verified)
#7 interdiff-6-7.txt1.04 KBAnonymous (not verified)
#7 2946425-7.patch20.43 KBAnonymous (not verified)
#6 interdiff-4-6.txt4.46 KBAnonymous (not verified)
#6 2946425-6.patch19.38 KBAnonymous (not verified)
#4 interdiff-2-4.txt5.27 KBAnonymous (not verified)
#4 2946419-4.patch32.64 KBAnonymous (not verified)
#2 2946419-2.patch11.86 KBAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

vaplas created an issue. See original summary.

Anonymous’s picture

Status: Active » Needs review
FileSize
11.86 KB

Status: Needs review » Needs work

The last submitted patch, 2: 2946419-2.patch, failed testing. View results

Anonymous’s picture

Status: Needs work » Needs review
FileSize
32.64 KB
5.27 KB

Status: Needs review » Needs work

The last submitted patch, 4: 2946419-4.patch, failed testing. View results

Anonymous’s picture

Status: Needs work » Needs review
FileSize
19.38 KB
4.46 KB
Anonymous’s picture

Title: Module: Convert system functional tests to phpunit » Entity: Convert system functional tests to phpunit
FileSize
20.43 KB
1.04 KB

Also added \Drupal\comment\TestsCommentCacheTagsTest from scope of #2862508: Convert system functional tests to phpunit. And this is stated in the #2866513: Convert web tests to browser tests for comment module.

Lendude’s picture

Status: Needs review » Needs work

@vaplas uhh just wondering why this went from being the 'module' conversion to being the 'entity' conversion? And why the patch number uses the issue number for #2946425: Correcting the tests that lie in BTB but using WTB, doing too many things at the same time again? :)

This looks great for the entity conversion. Nice and clean. One thing:

+++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
@@ -189,6 +189,8 @@ public static function getSkippedDeprecations() {
+      'Drupal\system\Tests\Entity\EntityDefinitionTestTrait is deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0. Instead, use \Drupal\Tests\system\Functional\Entity\Traits\EntityDefinitionTestTrait. See https://www.drupal.org/node/2946549.',
+      'Drupal\system\Tests\Entity\EntityWithUriCacheTagsTestBas is deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0. Instead, use \Drupal\Tests\system\Functional\Entity\EntityWithUriCacheTagsTestBas. See https://www.drupal.org/node/2946549.',

We should never add new deprecations to this, just take them out. Are there any uses of this left in core after this conversion? If so, why? If not, there is no reason to add them, plus there is a typo in the last one, so it won't work anyway :).

Anonymous’s picture

@Lendude, it was not my star day)))

#2822382: Make every $modules property protected on classes extending BrowserTestBase and KernelTestBase found 13 tests WTB tests in BTB location. I wanted to fix them all in one file, but then I guessed to file the Entity cases to a separate issue (🏅), because to solve them it was necessary to convert part system/entity.

Are there any uses of this left in core after this conversion? If so, why?

+ 'Drupal\system\Tests\Entity\EntityDefinitionTestTrait is deprecated in Drupal 8.5.0
It was a real wtf moment for me. I can not explain it, but without this change all tests with $modules = ['simpletest'] are fail. See #4 (and current test-only). Despite the fact that no one uses it.

Debug of OtherInstallationProfileTestsTest says that EntityDefinitionTestTrait it is requested only once in:

# /core/modules/simpletest/src/Form/SimpletestTestForm.php
// Generate the list of tests arranged by group.
$groups = $this->testDiscovery->getTestClasses(); # 153 line

...
# /core/modules/simpletest/src/TestDiscovery.php
$classmap = $this->findAllClassFiles($extension); # find traits too!
...
$info = static::getTestInfo($classname, $parser->getDocComment()); # line 181 
...
public static function getTestInfo($classname, $doc_comment = NULL) {
...
  $reflection = new \ReflectionClass($classname); # line 335.

But it seems this does not apply to EntityWithUriCacheTagsTestBase (after your fair point about typo :)

Anonymous’s picture

Reupload 2946419-9.patch with diff --find-copies.

The last submitted patch, 9: 2946419-9-test-only.patch, failed testing. View results

Anonymous’s picture

Anonymous’s picture

zymbian’s picture

Patch looks good to me , thanks for the patch.

Lendude’s picture

Status: Needs review » Postponed
Issue tags: +phpunit initiative

Lets postpone on #2949363: Impossible to make trigger_error in some files without test fails, because deprecating the trait is the right thing to do and we really don't want to add to that list of suppressed deprecation warnings.

Mile23’s picture

Status: Postponed » Needs work

EntityDefinitionTestTrait is loaded during test discovery. Since @trigger_error() is in the file, it is not triggered when the trait is used, only when the file is loaded.

So this means that if any test discovery system scans the file system in that directory, it's going to end up triggering the error if it requires, includes or uses reflection on that trait.

There is the Traits directory, but since we're trying to deprecate a trait in-place, that won't work. Traits in the Traits directory can be autoloaded, but won't scanned for tests. You can see an example of this in simpletest module: http://cgit.drupalcode.org/drupal/tree/core/modules/simpletest/tests/src

This is also a bit of an argument in favor of #2296635: Evaluate performance impact of limiting test discovery to *Test.php filename suffix since that would filter out files ending in Trait.php.

The solution here is to not @trigger_error(), and to file a followup to eventually add it. Add a @todo in the deprecated trait.

Anonymous’s picture

@Mile23 thanks for review and clarification of what is happening here!

However, solution without @trigger_error may be unacceptable. Perhaps it's better to add a @todo to DeprecationListenerTraitline:

+++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
@@ -189,6 +189,7 @@ public static function getSkippedDeprecations() {
+      // @todo: Remove after https://www.drupal.org/project/drupal/issues/2296635.
+      'Drupal\system\Tests\Entity\EntityDefinitionTestTrait is deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0. Instead, use \Drupal\Tests\system\Functional\Entity\Traits\EntityDefinitionTestTrait. See https://www.drupal.org/node/2946549.',

At least if someone creates tests with deprecated trait, the IDE will help about it through a strike trait.

Or just wait #2296635: Evaluate performance impact of limiting test discovery to *Test.php filename suffix.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
18.19 KB
3.92 KB

#2949363: Impossible to make trigger_error in some files without test fails 🎉

Due to many changes in the DeprecationListenerTrait, we can fix fail like simple reroll. But I specifically added this line to manually remove it again :)

Also nit update: 8.5.0/8.5.x

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

I can't see anything wrong with this patch, looks very good.

Mile23’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update
Related issues: +#2977105: Incomplete functional conversion for NodeCacheTagsTest

Diggit. :-) Pretty close...

+++ b/core/modules/system/src/Tests/Entity/EntityDefinitionTestTrait.php
@@ -5,8 +5,13 @@
+ * @deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.0.
+ * Use \Drupal\Tests\system\Functional\Entity\Traits\EntityDefinitionTestTrait.

+++ b/core/modules/system/src/Tests/Entity/EntityWithUriCacheTagsTestBase.php
@@ -7,8 +7,13 @@
+ * @deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.0.
+ * Use \Drupal\Tests\system\Functional\Entity\EntityWithUriCacheTagsTestBase.

Add @see to the CR in these docblocks. Also, it's deprecated in 8.6.x now.

Adding 'needs IS update' because of this question from #8: "@vaplas uhh just wondering why this went from being the 'module' conversion to being the 'entity' conversion?"

Adding #2977105: Incomplete functional conversion for NodeCacheTagsTest as related. We might end up closing that one if this one gets all the things.

Lendude’s picture

@Mile23 thanks for the review!

Updated the IS a bit, but the answer to #8 was in #9, it was a failed clone attempt :)

Updated the version numbers and added the needed @see's. Interdiff is a little weird, but that tends to happen with --find-copy patches.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

LGTM.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 2946419-21.patch, failed testing. View results

phenaproxima’s picture

Lendude’s picture

Status: Needs work » Reviewed & tested by the community

random fail

borisson_’s picture

The random fail was in Drupal\Tests\media\FunctionalJavascript\MediaSourceFileTest, so not related to this testcode.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 646584b and pushed to 8.6.x. Thanks!

Can we also get a follow-up to fix PHPUnit tests that extend \Drupal\system\Tests\Entity\EntityCacheTagsTestBase since that is also WTB. This is the problem with run-tests.sh it's far too clever for its own boots automatically determining test types using reflection.

  • alexpott committed 646584b on 8.6.x
    Issue #2946419 by vaplas, Lendude, Mile23: Entity: Convert system...
Mile23’s picture

The follow-up was already related: #2977105: Incomplete functional conversion for NodeCacheTagsTest

I just marked it closed because this issue fixed it.

Mile23’s picture

A bit of a follow-up to show us where the problem lies: #2981678: Fail PHPUnit-based tests which still inherit from WTB/TB.

Status: Fixed » Closed (fixed)

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