Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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:
- core/modules/system/src/Tests/Entity
- few WTB tests in BTB location (see #2822382-55: Make every $modules property protected on classes extending BrowserTestBase and KernelTestBase fails)
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#21 | 2946419-21.patch | 18.29 KB | Lendude |
#21 | interdiff-2946419-18-21.txt | 5.11 KB | Lendude |
#18 | interdiff-12-18.txt | 3.92 KB | Anonymous (not verified) |
#18 | 2946419-18.patch | 18.19 KB | Anonymous (not verified) |
#12 | interdiff-10-12.txt | 1.34 KB | Anonymous (not verified) |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedvaplas created an issue. See original summary.
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #6
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedAlso 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.
Comment #8
Lendude@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:
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 :).
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commented@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.
+ '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 thatEntityDefinitionTestTrait
it is requested only once in:But it seems this does not apply to
EntityWithUriCacheTagsTestBase
(after your fair point about typo :)Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedReupload 2946419-9.patch with
diff --find-copies
.Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedOne more unrelated nit change due to #2948700: Casting $text value to string in responseContains/responseNotContains methods.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedFiled #2949363: Impossible to make trigger_error in some files without test fails for problem with unexpected deprecated trigger.
Comment #14
zymbian CreditAttribution: zymbian as a volunteer commentedPatch looks good to me , thanks for the patch.
Comment #15
LendudeLets 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.
Comment #16
Mile23EntityDefinitionTestTrait
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.
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commented@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 toDeprecationListenerTrait
line: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.
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commented#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
Comment #19
borisson_I can't see anything wrong with this patch, looks very good.
Comment #20
Mile23Diggit. :-) Pretty close...
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.
Comment #21
Lendude@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.
Comment #22
Mile23LGTM.
Comment #24
phenaproximaI'm fairly sure this is blocking #2976889: Add a trait to create media types in tests and replace MediaFunctionalTestCreateMediaTypeTrait.
Comment #25
Lenduderandom fail
Comment #26
borisson_The random fail was in
Drupal\Tests\media\FunctionalJavascript\MediaSourceFileTest
, so not related to this testcode.Comment #27
alexpottCommitted 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.
Comment #29
Mile23The follow-up was already related: #2977105: Incomplete functional conversion for NodeCacheTagsTest
I just marked it closed because this issue fixed it.
Comment #30
Mile23A bit of a follow-up to show us where the problem lies: #2981678: Fail PHPUnit-based tests which still inherit from WTB/TB.