Problem/Motivation

UserMailNotifyTest is Kernel test but for some reason wrongly lives under Functional tests namespace.

Proposed resolution

Move UserMailNotifyTest under Kernel tests namespace.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

Status: Active » Needs review
FileSize
1.9 KB

Patch.

Lendude’s picture

Status: Needs review » Needs work

Oops, nice catch.

Couple of things we can clean up/make better while we are touching this:

+++ b/core/modules/user/tests/src/Kernel/UserMailNotifyTest.php
@@ -69,8 +69,8 @@ public function testUserMailsSent($op, array $mail_keys) {
   public function testUserMailsNotSent($op, array $mail_keys) {

$mail_keys isn't used in testUserMailsNotSent so we can remove that.

Another good addition would be to add array keys to the array returned by userMailsProvider so we PHPUnit gives clearer feedback about which dataset it is working on.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
3.82 KB
2.52 KB

Agree.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

Nice, looks ready to me.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed d88a43d47f to 8.8.x and 4c59167cb5 to 8.7.x. Thanks!

As a test only change backported to 8.7.x - I discussed backporting test-only changes with @catch (as a release manager).

+++ b/core/modules/user/tests/src/Kernel/UserMailNotifyTest.php
@@ -47,12 +71,12 @@ public function userMailsProvider() {
-    $this->assertTrue($return, '_user_mail_notify() returns TRUE.');
+    $this->assertTrue($return);

Personally I think for assertNull and assertTrue messages are a bit more useful but also at the end of the day if an assertion fails you always have to look at the test so /shrug.

  • alexpott committed d88a43d on 8.8.x
    Issue #3041816 by claudiu.cristea, Lendude: Move UserMailNotifyTest...

  • alexpott committed 4c59167 on 8.7.x
    Issue #3041816 by claudiu.cristea, Lendude: Move UserMailNotifyTest...

Status: Fixed » Closed (fixed)

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