Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#48 | 3028712-48.patch | 18.86 KB | Lendude |
#48 | interdiff-3028712-44-48.txt | 4.3 KB | Lendude |
#44 | 3028712-44.patch | 19.07 KB | Manuel Garcia |
#44 | interdiff-3028712-42-44.txt | 3.88 KB | Manuel Garcia |
#42 | 3028712-42.patch | 17.96 KB | Manuel Garcia |
Comments
Comment #2
Mile23Comment #3
danharper CreditAttribution: danharper as a volunteer and at hrpr commentedI 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?
Comment #5
danharper CreditAttribution: danharper as a volunteer and at hrpr commentedRE-rolled patch, created against wrong branch.
Comment #6
afonseca210 CreditAttribution: afonseca210 commenteddrupal blocking website from CDC. how can I fix this?
Comment #7
danharper CreditAttribution: danharper as a volunteer and at hrpr commentedPatch now passing so just needs to be reviewed.
Comment #8
Mile23Thanks. Still needs to remove
simpletest_mail_alter()
.Comment #9
danharper CreditAttribution: danharper as a volunteer and at hrpr commentedOk, new patch with
simpletest_mail_alter()
removedComment #10
Mile23Thanks, 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
Needs a newline.
Comment #11
kostyashupenkoadded newline and interdiff between #6 and #11
Comment #12
Mile23Thanks! Not quite there, though... I should have caught this before.
The test should not depend on the simpletest module.
Comment #13
danharper CreditAttribution: danharper as a volunteer and at hrpr commentedHi,
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
Comment #14
Mile23The goal is to remove dependencies on the simpletest module. So yes, please change all the test functions. :-)
Rescoping a little.
Comment #16
vadim.hirbu CreditAttribution: vadim.hirbu at FFW commentedRemoved dependencies to simpletest module and added interdiff between #11 and #16.
Comment #18
vadim.hirbu CreditAttribution: vadim.hirbu at FFW commentedUpdated latest patch to skip fail test on d.org.
Comment #19
vadim.hirbu CreditAttribution: vadim.hirbu at FFW commentedComment #21
vadim.hirbu CreditAttribution: vadim.hirbu at FFW commentedUpdated patch after excluding failing part related to simpletest strings. Added interdiff between #11 and #21.
Comment #22
claudiu.cristeaFor 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.
Comment #23
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedHere is a start at making the test a kernel test.
Comment #25
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedA 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.
Comment #27
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedSome more work on the tests, 2 still failing locally.
Comment #28
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedWe should probably change this email to something else.
Comment #30
claudiu.cristeaThe patch doesn't apply anymore. Needs reroll.
s/public/protected
Let's order this list alphabetically.
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.
Comment #31
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedJust a reroll for now, 3 way merge did the trick.
Thanks for the review @claudiu.cristea - I'll try tackling your comments now.
Comment #32
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedRe #30:
1. Done.
2. Done.
3. Done - also scanned the rest of the file and fixed it on a few other places.
Comment #33
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedOops
Comment #34
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedComment #36
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedA bit of progress, this should get rid of:
It'll still come back red though, couldn't figure out the 2 failures so far.
Comment #37
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedIf it helps, this is what I see locally:
Comment #39
mikelutzHere 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.
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.
Comment #40
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThanks 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
formailtest@example.com
while I was at it.Comment #41
mikelutzJust a couple small nits
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.
Let's go with
"KernelTestBase enforces the usage of the 'test_mail_collector' plugin to collect mail."
Comment #42
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThanks @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
Comment #44
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedOK maybe that wasnt such a good idea, but well I learned somethings along the way.
Reverting here to using globals, removing the todos
Comment #45
LendudeLooks ready to me, yeah the globals aren't pretty but *shrug*
Comment #47
larowlanCan 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.
Comment #48
LendudeFollow 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....
Comment #49
mikelutzI 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.
Comment #50
larowlanComment #51
larowlanCommitted 45ff41d and pushed to 8.8.x. Thanks!