Problem/Motivation

Drupal\Tests\system\Functional\Mail\MailTest() has a number of test functions that depend on the simpletest module.

The test should not depend on simpletest, since it's a BrowserTestBase test, and since we want to get rid of simpletest.

Proposed resolution

Add a fixture module to the system module's tests that will implement hook_mail_alter(), and have the test depend on that.

Remove simpletest_mail_alter().

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#48 3028712-48.patch18.86 KBLendude
#48 interdiff-3028712-44-48.txt4.3 KBLendude
#44 3028712-44.patch19.07 KBManuel Garcia
#44 interdiff-3028712-42-44.txt3.88 KBManuel Garcia
#42 3028712-42.patch17.96 KBManuel Garcia
#42 interdiff-3028712-40-42.txt4.04 KBManuel Garcia
#40 3028712-40.patch19.23 KBManuel Garcia
#40 interdiff-3028712-36-40.txt5.07 KBManuel Garcia
#36 3028712-36.patch17.69 KBManuel Garcia
#36 interdiff-3028712-33-36.txt1.46 KBManuel Garcia
#33 3028712-33.patch16.62 KBManuel Garcia
#33 interdiff-3028712-32-33.txt636 bytesManuel Garcia
#32 3028712-32.patch16.62 KBManuel Garcia
#32 interdiff-3028712-31-32.txt6.19 KBManuel Garcia
#31 3028712-31.patch14.32 KBManuel Garcia
#27 3028712-27.patch14.25 KBManuel Garcia
#27 interdiff-3028712-25-27.txt3.27 KBManuel Garcia
#25 3028712-25.patch11.95 KBManuel Garcia
#25 interdiff-3028712-23-25.txt997 bytesManuel Garcia
#23 3028712-23.patch11.93 KBManuel Garcia
#23 interdiff-3028712-21-23.txt3.16 KBManuel Garcia
#21 interdiff_11-21.txt6.39 KBvadim.hirbu
#21 test-cancel-message-cleanup-3028712-21.patch9.54 KBvadim.hirbu
#18 interdiff_11-18.txt8.11 KBvadim.hirbu
#18 test-cancel-message-cleanup-3028712-18.patch11.21 KBvadim.hirbu
#16 interdiff_11-16.txt8.29 KBvadim.hirbu
#16 test-cancel-message-cleanup-3028712-16.patch11.39 KBvadim.hirbu
#11 interdiff_6-11.txt961 byteskostyashupenko
#11 test-cancel-message-cleanup-3028712-11.patch3.25 KBkostyashupenko
#9 test-cancel-message-cleanup-3028712-9.patch3.29 KBdanharper
#5 test-cancel-message-cleanup-3028712-6.patch2.62 KBdanharper
#3 test-cancel-message-cleanup-3028712.patch3.45 KBdanharper
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Mile23’s picture

Issue tags: +Novice
danharper’s picture

Status: Active » Needs review
FileSize
3.45 KB

I think I've patched this correctly, just needs to be reviewed.

Created module for mail_cancel_test and updated MailTest class to use alter in mail_cancel_module.

There are many references to simpletest in the MailTest class, should these be logged separately?

Status: Needs review » Needs work

The last submitted patch, 3: test-cancel-message-cleanup-3028712.patch, failed testing. View results

danharper’s picture

RE-rolled patch, created against wrong branch.

afonseca210’s picture

drupal blocking website from CDC. how can I fix this?

danharper’s picture

Status: Needs work » Needs review

Patch now passing so just needs to be reviewed.

Mile23’s picture

Status: Needs review » Needs work

Thanks. Still needs to remove simpletest_mail_alter().

danharper’s picture

Ok, new patch with simpletest_mail_alter() removed

Mile23’s picture

Thanks, looks pretty good. :-)

It's helpful to have interdiffs. In this case the patch isn't so big but if it were complicated it would be easier to see. https://www.drupal.org/documentation/git/interdiff

+++ b/core/modules/system/tests/modules/mail_cancel_test/mail_cancel_test.module
@@ -0,0 +1,19 @@
\ No newline at end of file

Needs a newline.

kostyashupenko’s picture

Status: Needs work » Needs review
FileSize
3.25 KB
961 bytes

added newline and interdiff between #6 and #11

Mile23’s picture

Status: Needs review » Needs work

Thanks! Not quite there, though... I should have caught this before.

+++ b/core/modules/system/tests/src/Functional/Mail/MailTest.php
@@ -24,7 +24,7 @@ class MailTest extends BrowserTestBase {
+  public static $modules = ['simpletest', 'system_mail_failure_test', 'mail_cancel_test', 'mail_html_test', 'file', 'image'];

The test should not depend on the simpletest module.

danharper’s picture

Hi,

Sorry, that's what I meant in comment 3, there are more references to simpletest in that class in the other functions, so should we log a ticket for each function that needs to be changed or change the title of this bug to "MailTest cleanup" so that it covers all functions in that class?

Cheers Dan

Mile23’s picture

Title: Drupal\Tests\system\Functional\Mail\MailTest::testCancelMessage() cleanup » Drupal\Tests\system\Functional\Mail\MailTest simpletest dependency cleanup
Issue summary: View changes

The goal is to remove dependencies on the simpletest module. So yes, please change all the test functions. :-)

Rescoping a little.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

vadim.hirbu’s picture

Status: Needs review » Needs work

The last submitted patch, 16: test-cancel-message-cleanup-3028712-16.patch, failed testing. View results

vadim.hirbu’s picture

Status: Needs work » Needs review
FileSize
11.21 KB
8.11 KB

Updated latest patch to skip fail test on d.org.

vadim.hirbu’s picture

Status: Needs review » Needs work

The last submitted patch, 18: test-cancel-message-cleanup-3028712-18.patch, failed testing. View results

vadim.hirbu’s picture

Status: Needs work » Needs review
FileSize
9.54 KB
6.39 KB

Updated patch after excluding failing part related to simpletest strings. Added interdiff between #11 and #21.

claudiu.cristea’s picture

Title: Drupal\Tests\system\Functional\Mail\MailTest simpletest dependency cleanup » Convert system MailTest into a Kernel test. Break its simpletest dependency
Component: simpletest.module » mail system
Status: Needs review » Needs work
Issue tags: -Novice +Test suite performance
Parent issue: » #3041700: [META] Convert some tests into Kernel or Unit tests

For me this test look like a Kernel test. Let's convert it here. Please provide also some metrics on the time we save with the conversion.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
3.16 KB
11.93 KB

Here is a start at making the test a kernel test.

Status: Needs review » Needs work

The last submitted patch, 23: 3028712-23.patch, failed testing. View results

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
997 bytes
11.95 KB

A better starting point.

Tests however fail locally, only one is passing. Looks to me like something having to do with setting up the default mail interface, ran out of time before figuring it out.

Status: Needs review » Needs work

The last submitted patch, 25: 3028712-25.patch, failed testing. View results

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
3.27 KB
14.25 KB

Some more work on the tests, 2 still failing locally.

Manuel Garcia’s picture

+++ b/core/modules/system/tests/src/Kernel/Mail/MailTest.php
@@ -87,20 +104,20 @@ public function testFromAndReplyToHeader() {
     $from_email = 'Drupal <simpletest@example.com>';

We should probably change this email to something else.

Status: Needs review » Needs work

The last submitted patch, 27: 3028712-27.patch, failed testing. View results

claudiu.cristea’s picture

Issue tags: +Needs reroll

The patch doesn't apply anymore. Needs reroll.

  1. +++ b/core/modules/system/tests/src/Kernel/Mail/MailTest.php
    @@ -17,28 +17,47 @@
    +  public static $modules = [
    

    s/public/protected

  2. +++ b/core/modules/system/tests/src/Kernel/Mail/MailTest.php
    @@ -17,28 +17,47 @@
    +    'system_mail_failure_test',
    +    'mail_cancel_test',
    +    'mail_html_test',
    +    'file',
    +    'image',
    +    'user',
    

    Let's order this list alphabetically.

  3. +++ b/core/modules/system/tests/src/Kernel/Mail/MailTest.php
    @@ -17,28 +17,47 @@
    -    $this->assertTrue($mail_backend instanceof TestPhpMailFailure, 'Default mail interface can be swapped.');
    +    $this->assertInstanceOf(TestPhpMailFailure::class, $mail_backend, 'Default mail interface can be swapped.');
    
    @@ -48,24 +67,22 @@ public function testPluggableFramework() {
    -    $this->assertTrue($mail_backend instanceof TestMailCollector, 'Additional mail interfaces can be added.');
    +    $this->assertInstanceOf(TestMailCollector::class, $mail_backend, 'Additional mail interfaces can be added.');
    
    @@ -87,20 +104,20 @@ public function testFromAndReplyToHeader() {
    -    $this->assertEqual($from_email, $sent_message['headers']['From'], 'Message is sent from the site email account.');
    -    $this->assertEqual($reply_email, $sent_message['headers']['Reply-to'], 'Message reply-to headers are set.');
    +    $this->assertEquals($from_email, $sent_message['headers']['From'], 'Message is sent from the site email account.');
    +    $this->assertEquals($reply_email, $sent_message['headers']['Reply-to'], 'Message reply-to headers are set.');
         $this->assertFalse(isset($sent_message['headers']['Errors-To']), 'Errors-to header must not be set, it is deprecated.');
    

    In Simpletest, the assertion message has described the success. Contrary, in PHPUnit the assertion messages are describing the failure. A good practice would be to remove them from assertion an move them as comments just before the assertion line.

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia
Issue tags: -Needs reroll
FileSize
14.32 KB

Just a reroll for now, 3 way merge did the trick.

Thanks for the review @claudiu.cristea - I'll try tackling your comments now.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
6.19 KB
16.62 KB

Re #30:

1. Done.
2. Done.
3. Done - also scanned the rest of the file and fixed it on a few other places.

Manuel Garcia’s picture

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned

Status: Needs review » Needs work

The last submitted patch, 33: 3028712-33.patch, failed testing. View results

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
1.46 KB
17.69 KB

A bit of progress, this should get rid of:

1) Drupal\Tests\system\Kernel\Mail\MailTest::testErrorMessageDisplay
Undefined offset: 0

/var/www/html/core/modules/system/tests/src/Kernel/Mail/MailTest.php:86

It'll still come back red though, couldn't figure out the 2 failures so far.

Manuel Garcia’s picture

If it helps, this is what I see locally:

There were 2 failures:

1) Drupal\Tests\system\Kernel\Mail\MailTest::testFromAndReplyToHeader
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Drupal <simpletest@example.com>'
+' <>'

/var/www/drupal8/core/tests/Drupal/KernelTests/KernelTestBase.php:1111
/var/www/drupal8/core/modules/system/tests/src/Kernel/Mail/MailTest.php:138

2) Drupal\Tests\system\Kernel\Mail\MailTest::testRenderedElementsUseAbsolutePaths
Asserting that root relative paths are converted properly.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-<img src="http://localhost//o2OYBPUi.png" alt="" />
+<img 
+src="http://localhost/vfs://root/sites/simpletest/82770968/files/o2OYBPUi.png" 
+alt="" />

/var/www/drupal8/core/modules/system/tests/src/Kernel/Mail/MailTest.php:333

Status: Needs review » Needs work

The last submitted patch, 36: 3028712-36.patch, failed testing. View results

mikelutz’s picture

  1. +++ b/core/modules/system/tests/src/Kernel/Mail/MailTest.php
    @@ -137,21 +165,24 @@ public function testFromAndReplyToHeader() {
    -    \Drupal::service('plugin.manager.mail')->mail('simpletest', 'from_test', 'from_test@example.com', $language);
    +    \Drupal::service('plugin.manager.mail')->mail('mail_cancel_test', 'from_test', 'from_test@example.com', $language);
    

    Here you will need to set up the system.site config to have site name and mail, because the functional setup site install is not happening to do it for you.

  2. +++ b/core/modules/system/tests/src/Kernel/Mail/MailTest.php
    @@ -248,8 +282,11 @@ public function testConvertRelativeUrlsIntoAbsolute() {
    @@ -284,7 +321,7 @@ public function testRenderedElementsUseAbsolutePaths() {
    
    @@ -284,7 +321,7 @@ public function testRenderedElementsUseAbsolutePaths() {
           ];
           $expected_html = "<img src=\"$expected_path\" alt=\"\" />";
    

    This is from not having a proper public site files path set in the site.path service. I'm not 100% how best to mock that here.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
5.07 KB
19.23 KB

Thanks so much @mikelutz for the tips on this, they really helped!

#39.1 was simple enough to fix, thanks

#39.2 is a lot trickier... and I am not 100% convinced with what I'm proposing here... That said, I think it's enough for what we're trying to test here?

Also switched simpletest@example.com for mailtest@example.com while I was at it.

mikelutz’s picture

Status: Needs review » Needs work

Just a couple small nits

  1. +++ b/core/modules/system/tests/src/Kernel/Mail/MailTest.php
    @@ -18,28 +19,48 @@
    +    // @todo can this be avoided?
    
    @@ -57,7 +79,11 @@ public function testPluggableFramework() {
    +    // @todo can this be avoided?
    
    @@ -160,8 +198,11 @@ public function testFromAndReplyToHeader() {
    +    // @todo can this be avoided?
    
    @@ -248,8 +289,11 @@ public function testConvertRelativeUrlsIntoAbsolute() {
    +    // @todo can this be avoided?
    

    I don't know the answer to this question, but lets decide the answer now. If it can be avoided, lets create a linked follow-up in the @todo, and if it can't/shouldn't lets remove the @todo.

  2. +++ b/core/modules/system/tests/src/Kernel/Mail/MailTest.php
    @@ -18,28 +19,48 @@
    +    // KernelTestBase enforces the usage of 'test_mail_collector' plugin for
    +    // collect the mails. Since we need to test this functionality itself, we
    
    @@ -57,7 +79,11 @@ public function testPluggableFramework() {
    +    // KernelTestBase enforces the usage of 'test_mail_collector' plugin for
    +    // collect the mails. Since we need to test this functionality itself, we
    
    @@ -160,8 +198,11 @@ public function testFromAndReplyToHeader() {
    +    // KernelTestBase enforces the usage of 'test_mail_collector' plugin for
    +    // collect the mails. Since we need to test this functionality itself, we
    
    @@ -248,8 +289,11 @@ public function testConvertRelativeUrlsIntoAbsolute() {
    +    // KernelTestBase enforces the usage of 'test_mail_collector' plugin for
    +    // collect the mails. Since we need to test this functionality itself, we
    

    Let's go with
    "KernelTestBase enforces the usage of the 'test_mail_collector' plugin to collect mail."

  3. I think the solution to #39.2 is fine for this case, I think the point is that we are rendering with absolute urls and not relative, which we are.
Manuel Garcia’s picture

Thanks @mikelutz for the review.

Re #41.1
I searched around a bit, and can see some effort going on around this #1956180: [meta] Remove global variables and #2183323: Replace $GLOBALS['conf']['container_service_providers'] in DrupalKernel with Settings

I also had a bit of a play with this, changed the line in KernelTestBase to this:
$this->config('system.mail')->set('interface.default', 'test_mail_collector')->save(); and used the same approach on MailTest to setup our on mail interface, tests passed, so I think we can actually use this approach.

Opened #3059285: Replace $GLOBALS['config']['system.mail']['interface']['default'] in DrupalKernel with config for this, and the uploaded patch includes it (to see what the bot says).

Re #41.2 Done

Status: Needs review » Needs work

The last submitted patch, 42: 3028712-42.patch, failed testing. View results

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
3.88 KB
19.07 KB

OK maybe that wasnt such a good idea, but well I learned somethings along the way.

Reverting here to using globals, removing the todos

Lendude’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Drupal 9

Looks ready to me, yeah the globals aren't pretty but *shrug*

The last submitted patch, 31: 3028712-31.patch, failed testing. View results

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/system/tests/src/Kernel/Mail/MailTest.php
@@ -248,8 +287,10 @@ public function testConvertRelativeUrlsIntoAbsolute() {
+    // KernelTestBase enforces the usage of 'test_mail_collector' plugin to
+    // collect mail. Since we need to test this functionality itself, we
+    // manually configure the default mail interface.
+    $GLOBALS['config']['system.mail']['interface']['default'] = 'test_html_mail_collector';

Can we get a follow-up to make KernelTestBase support the mail handler as a property? eg $this->mailHandler

And a to-do to that follow-up for these?

Thanks, there's not much more we can do here, but having that as a property means its at least swappable without (more) globals.

Lendude’s picture

Follow up : #3076715: Make KernelTestBase support the mail handler as a property

Moved the setting of the mail interface to a method on the test class so we don't have to have the comment/@todo/$GLOBAL in four different spots.

Edit: no idea why the interdiff is showing changes to simpletest_mail_alter, the new patch doesn't change anything there....

mikelutz’s picture

Status: Needs review » Reviewed & tested by the community

I think there's been some commits in that file, so your the function location moved. Not sure exactly why it showed up in the interdiff though, I can't think of a rebase/merge/diff scenario that would cause that.

Anyway, the patch looks good. Feedback addressed, and +1 for consolidating the nastiness.

larowlan’s picture

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed 45ff41d and pushed to 8.8.x. Thanks!

  • larowlan committed 45ff41d on 8.8.x
    Issue #3028712 by Manuel Garcia, vadim.hirbu, danharper, Lendude,...

Status: Fixed » Closed (fixed)

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