[Plan] Remove the usage of deprecated methods in tests

This needs to be scripted and automated. And ideally we need to cope with removing the test messages but that's where things can tricky. The per module scope never works and it always the wrong choice. I think the best and most manageable scope is per method - where each issue adds an @trigger_error() to each method as well and then we can be sure we don't regress.

I also think we should consider only deprecating this stuff in Drupal 10 because the ability to change test classes from WebTestBase to BrowserTestBase and only have to make minimal changes is good.

Note this does not mean we shouldn't do the replacements in core it just means we should consider leaving the traits in for a bit longer because they do not have the same burden of support that WebTestBase has and in fact could help contrib / custom move to Drupal 9 with less of a bump.

#2/#3 ++

Leaving things like \Drupal\FunctionalTests\AssertLegacyTrait in D9 makes perfect sense to me, we should probably open an issue to change the deprecation message on that.

I do concur with #2#3#4. Having a different way for essentially the same assertions doesn't really add much value, but yeah doing the right thing in Drupal core itself, improves a bit the problem with decision paralysis when touching existing tests.

Replacing things per method seems to be absolutely the right thing to do.

To my reckoning, there are 59 occurrences of '@deprecated' in 9 files within core/tests/.

Drupal\FunctionalTests\AssertLegacyTrait: 4 out of 43 methods trigger an error. assertTextHelper() is not marked @deprecated. Also no change record.

Drupal\KernelTests\AssertLegacyTrait: 10 methods, none trigger any errors. assertTrue() and verbose() are not marked @deprecated.

BrowserTestBase::drupalGetHeaders() needs trigger.

PhpunitCompatibilityTrait::getMock() needs a trigger. #2929133: Replace all usages of getMock()

That's it. Here's a patch which adds generic @trigger_error() to functional AssertLegacyTrait to see which ones need love.

@Mile23 yeah but... Like \Drupal\FunctionalTests\AssertLegacyTrait::assertText() has around 2000 usages... so "That's it" is hiding quite a bit of work. Plus in this instance we need to remove $message because it is ignored. Also for things like \Drupal\KernelTests\AssertLegacyTrait::assertEqual() the order of arguments has swapped - ie.

  protected function assertEqual($actual, $expected, $message = '') {
    $this->assertEquals($expected, $actual, $message);
  }

And again we are in the realm of thousands of usages.

'That's all' isn't what I really meant to say. Just that it's basically <50 methods.

I didn't see any analysis so I started looking.

With #3031580: Undeprecate \Drupal\FunctionalTests\AssertLegacyTrait and \Drupal\KernelTests\AssertLegacyTrait in Drupal 8 in, this can be moved to D9 I think.

@Lendude I guess we can start of this anytime right? I guess this feels majorly disruptive but maybe someone can script stuff once we're at 0 simpletests and work on it if they like.

Tried to kick this off with #3129002: Replace usages of deprecated AssertLegacyTrait::assert() which confuses me every time I see it, because I always think "assert what exactly?" or get it confused with the PHP assert() function.

Referencing a related issue to linking in more easily; #3128573: [meta] Replace assertions with more appropriate ones.

Adding deprecation silencers.

edit: wrong issue

Filed #3135077: Remove usage of AssertLegacyTrait::pass() from traits

Adding related that also impacts deprecation of assertion methods.

Adding a related issue #3142267: Allow the full list of deprecations from PHPUnit8Warning to respect the --suppress-deprecations behavior on DrupalCI, which might affect here

Hi @jungle, how about adding this issues to the nice kanban board you created for #3128573: [meta] Replace assertions with more appropriate ones?

Hi, @mondrake, I would create a tag-based kanban board for this, instead of adding issues one by one to the board like that one.
Adding a tag to the issue, it will show on the board automatically.

If you agree, which tag you prefer, Betterize Tests?

Let's try "Test improvements"

Betterize Tests

Ha! Tempting. Very tempting. Especially to me that developed a module named 'Pagerer', it being a 'better pager'...

But I doubt others would appreciate as much as I would...

If that's for this meta only, how about simply Deprecated assertions?

Well, here you go :)

So next, let's remove the issue listed in IS after adding the "Deprecated assertions" tag to them?

Finished adding the "Deprecated assertions" tag to child issues.

:( Not all issues are listed in the board, will check back, or report it to https://github.com/mglaman/contribkanban.com/issues

Thanks @jungle, great!

Hi @jungle the kanban board does not list postponed issues? is that expected?

Yes, it seems buggy, I am new to that site too. Some issues are missing still, and it does not fetch in realtime. With the tag and other filters, D.O.'s default issue queue looks great to me. Here is the link.

https://www.drupal.org/project/issues/search?issue_tags=Deprecated%20ass...

RFC: #3142806: Assertion naming collision between AssertContentTrait and AssertLegacyTrait

Thanks!

Should we do the following micro-optimization in each child issue if applicable? Or a new meta?

Before: Each $this->assertSession() call returns a new instance.

public function testFooBar() {
  $this->assertSession()->foo();
  $this->assertSession()->bar();
  $this->assertSession()->foo();
  $this->assertSession()->bar();
}

After:

public function testFooBar() {
  $assert_session = $this->assertSession();
  $assert_session->foo();
  $assert_session->bar();
  $assert_session->foo();
  $assert_session->bar();
}

Or improve it at $this->assertSession(), so we do not have to make changes in tests. returns one same instance if possible every time, instead of a new instance.

#45 personally I'm -1 on that, at this stage. IMHO I'd rather do the conversions first, then tackle that later, and that's not necessarily the only way to do it: for example, we could cache the WebAssert instance instead of creating a new one every time, letting assertSession return it, and maybe adding a reset method to be run when necessary. Or we could add methods like $this->assertSessionFoo() to reduce the piping.

EDIT - xpost with #46

I reviewed a few child issues that were RTBC. In both cases, although the patch dutifully removes the usages of deprecated test assertions in Functional tests, I keep finding un-deprecated versions of the same assertions in core/tests/Drupal/KernelTests/AssertContentTrait.php. :( That seems highly problematic. What's the story on that trait? Shouldn't we have deprecated all these same methods in there, too?

Thanks!
-Derek

#49 no, AssertContentTrait is a different implementation, not under deprecation. I did not follow the initial developments, but my undertsanding is that these methods were used in Simpletest, then when moved to PHPUnit, the Functional tests took Mink's WebAssert path with a proxy to the old methods' names in AsserLegacyTrait, and Kernel tests a separate implementation. Here we're removing the deprecated functional AssertLegacyTrait methods to leave only the WebAssert implementation.

There are other issues for AssertContentTrait.

See also:

• #3142749-27: AssertLegacyTrait::assertPattern() calls in functional tests still have a message passed in
• #3162193: Modernize AssertContentTrait
• #3142806: Assertion naming collision between AssertContentTrait and AssertLegacyTrait

Re #50: Okay, makes sense. Thanks for the links. I'll follow those.

Thanks again,
-Derek

Some of them were backported, some were not -- committed to 9.1.x only (For example: #3139433: Replace usages of AssertLegacyTrait::assert(No)Escaped, that is deprecated). Should all be backported to keep consistent?

#53 agree, it's a mishmash ATM. Personally, I would be in favor of closing the issues with commit to the latest dev branch only. Then, if someone needs, they can open a backport only issue. There are issues that are fixed now, and became stale in backports, that do not help in tracing progress. Also, strange situations like AssertLegacyTrait::pass removed from tests in 3 branches, but the issue to backport removal from traits is blocked at NW.

Personally, I would be in favor of closing the issues with commit to the latest dev branch only

+1

And +1 to mark those became stale in backports fixed ATM

Filed #3227265: Remove the legacy assert traits for D10.

I think this issue is ✅ now, the legacy assert traits can be removed in D10.

This was a big effort, nice to see it done in time for 10.0.x removal when it opens. Added some (somewhat arbitrary) issue credit since this is a meta for the planning/scoping work.