Problem/Motivation

Remove from testing framework (base test suite classes, traits, listeners, etc) the code that was deprecated in the D9 cycle for removal in D10.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3261266

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: Unassigned » mondrake

On this

mondrake opened merge request !1740

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Related issues: +#3254726: Remove SimpleTest support from run-tests.sh, TestDiscovery and TestRunnerKernel

Would appreciate a review of #3254726: Remove SimpleTest support from run-tests.sh, TestDiscovery and TestRunnerKernel if you have time.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: mondrake » Unassigned
Status: Active » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

One question about strictness following this patch.

Also should we remove AssertButtonsTrait here? Not sure where else we would do it:

core/modules/node/tests/src/Functional/AssertButtonsTrait.php:@trigger_error(__NAMESPACE__ . '\AssertButtonsTrait is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. There is no replacement. See https://www.drupal.org/node/3215411', E_USER_DEPRECATED);

Otherwise I think this covers all test-related code.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Thank you! Removed AssertButtonsTrait.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

OK, I can see the argument for keeping them to let contrib know, it only affects tests and will make them cleaner in the long run. Maybe we should flag these asserts for removal in 11.0.x and/or have a followup issue somewhere to do it?

Marking RTBC for core committer opinion.

catch’s picture

catch
he/him
Primary language English
commented

We already trigger deprecation errors where the extra argument is passed to those methods, so I think we could probably go ahead and remove the overrides altogether.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
commented

rerolled

daffie’s picture

daffie commented
Status: Needs review » Needs work

Just 1 remark.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

So, #10 suggests to remove the overrides, and #13 suggests to keep them but change from assert() to \InvalidArgumentException.

Now, in PHPStan level-1 we will have a static analysis check to ensure that a function is not called with more arguments than it can handle, which is IMHO the best solution.

This is what e.g. you get running PHPStan with level 1:

+		-
+			message: "#^Method PHPUnit\\\\Framework\\\\Assert\\:\\:assertSame\\(\\) invoked with 4 parameters, 2\\-3 required\\.$#"
+			count: 1
+			path: modules/block_content/tests/src/Kernel/Views/RevisionUserTest.php
+

because in that test we have this code in the UserCreationTest

    $this->assertSame(SAVED_NEW, $result, new FormattableMarkup('Created role ID @rid with name @name.', ['@name' => var_export($role->label(), TRUE), '@rid' => var_export($role->id(), TRUE)]), 'Role');

That is we have the $group argument which is cruft from Simpletest.

But, we are still quite far from level-1.

IMHO we better leave some checks to ensure we do not add extra arguments, so that when we move to level-1 we do not find more assertions to change.

So, I still stand behind the patch as is now.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

@mondrake: Ok, lets go with the assertions.

All changes look good to me.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Hmm. So personally I would not have added the assertions, and would have removed the test coverage for the extra arguments, given that phpstan with (eventually) cover this. However it is not doing any harm, and is a step forward overall, so committed/pushed to 10.0.x, thanks!

  • catch committed 6c97da6 on 10.0.x 
    Issue #3261266 by mondrake, longwave, daffie: Remove deprecated code...
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Reviewed & tested by the community » Fixed

Somehow 'Fixed' did not stick in #16.

mondrake closed merge request !1740

Status: Fixed » Closed (fixed)

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