Closed (fixed)
Project:
Drupal core
Version:
9.2.x-dev
Component:
phpunit
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
4 Sep 2020 at 16:20 UTC
Updated:
1 Feb 2021 at 16:14 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
longwaveDeprecated the trait, but this breaks the test; the autoloader triggers the deprecation and I can't figure out how to add an expectation for it.
Also spotted a typo in the existing AssertHelperTraitTest, probably should spin that out to a new issue.
Comment #4
anmolgoyal74 commentedI am not sure if this works.
Comment #5
mondrake@anmolgoyal74 thanks but no, it doesn't... With deprecations we should not refactor the deprecated code, just add the deprecation elements, https://www.drupal.org/core/deprecation
@longwave re #2 I am afraid we cannot add a @trigger_error at the trait level here, nor we would be able to do so with any test class or test trait. The problem lies in the test discovery process, as the PhpUnitCliTest failure shows in the test-only patch here - AssertHelperTraitTest.php is found during the directory scan, its class Reflected to find the test methods, during reflection the trait is loaded and the @trigger_error fires - hence the deprecation exception captured. Unless we get to find a way to have a mechanism that silences all deprecations catered during the test discovery phase, we are not able to workaround that.
Maybe for a test trait it's sufficient to add @deprecated and skip the @trigger_error?
Comment #6
mondrakeUh, maybe this works - moving the AssertHelperTestClass out of the discoverable tests namespaces. It's a bit byzantine though. Thanks @anmolgoyal74 for the inspiration from patch in #4.
Comment #7
mondrakeWith a test method testing the trait's deprecation. We cannot add the @expectedDeprecation to the already exixsting method - since it's a multiple test supported by a data provider, only the first instance would intercept the deprecation since the error is triggered only once at class loading.
Comment #9
mondrakeAdjusted issue title and summary to cover scope per #2, let's not split further.
Comment #10
longwaveThanks! This solution works and while it is a bit convoluted, it is only in test code that is to be removed so I don't see any issue with it. It also provides a pattern for us to use again if we need to.
Comment #12
mondrake#11 is a random test failure
Comment #13
alexpottLet's not put this in TestTools - it's not necessary as far as I know. And this is not a test tool. I think this can go in core/tests/Drupal/Tests/fixtures.
Comment #14
ayushmishra206 commentedMade the changes asked in #13. Please review.
Comment #15
mondrake@ayushmishra206 that seems a bit too far :) The class is necessary, just it's not necessary to reside in the
Drupal\TestToolsnamespace.Comment #16
ayushmishra206 commentedI am sorry, please ignore the previous patch.
Comment #17
alexpottWe need to move the class to core/tests/Drupal/Tests/fixtures (note the directory needs to be created) and the namespace needs to be changed to namespace Drupal\Tests\fixtures; and therefore the use needs to be changed.
You can run core/tests/Drupal/Tests/AssertHelperTraitTest.php test before uploading to check you have everything correct.
Comment #18
mondrake#17 hmm wouldn't the
Drupal\Tests\fixturesnamespace be in the domain of those discoverable for testing? If so, we'd end up with the same problem.Comment #19
mondrakeOn this.
Comment #20
mondrakeMoving the class file to the core/tests/fixtures directory instead (which is out of the test discovery path), and loading the class with a require_once in the first test.
Comment #21
mondrakeLooking at this again, I am actually afraid now that this change
in the parent issue is breaking BC :(
Comment #23
daffie commentedComment #24
ankithashettyRerolled the patch in #20. Thanks!
Comment #25
daffie commented@ankithashetty: You missed this change is your reroll.
Comment #26
ankithashettyHi @daffie, the code that you mentioned in #25 has been updated in this issue #3172438: Since symfony/phpunit-bridge 5.1: Using "@expectedDeprecation" annotations in tests is deprecated, use the "ExpectDeprecationTrait::expectDeprecation()" method instead.. So, I think it's no longer required. Updated the patch accordingly. Thank you...
Comment #27
sulfikar_s commentedHi,
I've applied the patch in #26 and it applied cleanly. But the file(core/tests/fixtures/AssertHelperTestClass.php) which is created as being part of this is having a 'use' statement that is not being used. which caused the Custom Commands Failure.
I removed that line. Please review.
Comment #28
daffie commentedThe deprecation in the patch needs to be updated to 9.2.
Comment #29
sulfikar_s commentedHi, Corrected that to 9.2.
Please review.
Comment #30
daffie commentedYou missed one.
Comment #31
Pooja Ganjage commentedHi,
Creating a patch as suggested in #30 comment.
Please review the patch.
Thanks.
Comment #32
Pooja Ganjage commentedComment #34
anmolgoyal74 commentedFixed the failing test.
Comment #35
daffie commentedThe trait
Drupal\Tests\AssertHelperTraithas only one method and we are deprecating that method.Therefore we can deprecate the whole trait and that is what is patch does.
There is a deprecation message test added to the patch.
There is also a CR.
For me it is RTBC.
Comment #37
catchCommitted 3302b4e and pushed to 9.2.x. Thanks!