Closed (fixed)
Project:
Drupal core
Version:
9.1.x-dev
Component:
phpunit
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
23 May 2020 at 12:28 UTC
Updated:
4 Jun 2021 at 16:49 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
jungleAdding the "Deprecated assertions" tag to add this into the kanban board https://contribkanban.com/board/Deprecatedassertions
Comment #3
munish.kumar commentedComment #4
munish.kumar commentedComment #5
munish.kumar commentedFixed linting issues.
Comment #7
pavnish commentedComment #8
pavnish commentedThis is the testing patch only
Comment #10
pavnish commentedComment #11
mondrakeThanks for working on this
This is a very big elephant, that will be probably have to be sliced for eating. IMHO we should wait for most of the #3027952: [Plan] Remove the usage of deprecated methods in tests sub-issues to be done before, maybe also for #2549805: [Meta] Remove all usage of FormattableMarkup in tests apart from explicit tests of that API. Then find ways to break this down into smaller patches (e.g. separate assertText from assertNoText, split base classes+traits from concrete test, etc).
Comment #12
mondrakeSimilarly to #3139409: Replace usages of AssertLegacyTrait::assertRaw, that is deprecated, I would suggest splitting this issue in 3:
1) a first issue to remove the
$messageparameter from calls toassertTextandassertNoText. This is already redundant as in the legacy trait these methods are defined asprotected function assertText($text). Suggest following #3131946: [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages for guidelines when a $message should be converted to an inline comment.2) a second issue to remove the calls to t() from calls to
assertText, unless we are testing the translation layer.3) finally this one that at that point will be simply a find and replace exercise.
Comment #13
mondrakeFiled #3159788: AssertLegacyTrait::assert(No)Text() in functional tests still have a message passed in
Comment #14
pavnish commentedIt's postponed so i am unassign this issue
Comment #15
mondrakeComment #16
mondrakeComment #18
mondrakeComment #19
mondrakeI am splitting this in two, one for
assertTextand one forassertNoText, since the conversions are not as straightforward as it may seem.Comment #21
mondrakeComment #22
mondrakeComment #23
mondrakeGreen! Ready for review.
Comment #24
daffie commented@mondrake: When I download the MR as a patchfile it is 850 KB. Should we schedule this issue in the same way as #3186661: Remove usage of drupalPostForm?
Comment #25
mondrake@daffie re. #24 obviously, this is a big patch. I'd leave that decision to committers. Personally, I respect the need to minimise the impact of large changes. OTOH, I see that having only a 10 days window every 6 months to get those done, the risk is that it will take a long time to do them especially if you need to start doing a big one only after another big one is committed. For example, these remaining Simpletest deprecations
are all pretty large, interdependent one to the other, and non-trivial (it's not just renaming the method, we need to check order of arguments, select the appropriate method in some cases, etc...).
Comment #26
daffie commented@mondrake: If you have a good idea how we can get this done in another way, then please say so. Because I do not.
Comment #27
mondrakeSame here, @daffie :)
Comment #28
daffie commentedComment #29
mondrakeRerolled
Comment #30
mondrakeComment #31
longwaveComment #33
mondrakeComment #35
longwaveComment #36
mondrakeRerolled to solve conflicts introduced by commit of #3131281: Replace assertEqual() with assertEquals().
Comment #37
longwaveAgreed with @mondrake in Slack that we both think each other's commits here are good, so we are joint-RTBC-ing this patch together :)
Comment #38
mondrakeConfirmed, +1 for RTBC
https://drupal.slack.com/archives/C1BMUQ9U6/p1621515755041300
Comment #39
alexpottCommitted and pushed 3872f1a8ed to 9.3.x and 01d96560b5 to 9.2.x. Thanks!
Let's backport this without the changes to skipped deprecations.
Comment #44
longwaveComment #45
mondrakeComment #46
daffie commentedI am reviewing
Comment #47
alexpottThis one needs a re-roll - the other disruptive patches have landed in 9.1.x
Comment #48
daffie commentedI shall wait for the reroll.
Comment #49
longwaveMerged in 9.1.x and fixed conflicts.
Comment #50
mondrakeRTBC; as far as I can see, this is only removing lines with
assertText(...)and adding lines withpageTextContains(...)orresponseContains(...).Working on #3191935: Replace usages of AssertLegacyTrait::assertNoText, which is deprecated I now realize that this conversion may be impacted by #3175718: Random fails due to drupal-settings-json being counted as page text in the sense that if an expected text is found in the drupal-settings-json but not in the user visible page, still the assertion would pass.
Comment #51
alexpottCommitted 5a6777a and pushed to 9.1.x. Thanks!