The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
While working on test coverage for #3078110: Notification emails should not send to blocked users I saw weird results at the test-only patch:
https://www.drupal.org/pift-ci-job/2139085
I was only expecting a single fail, but I got 2. The output shows the expected test failure, but twice.
Drupal\Tests\content_moderation_notifications\Kernel\TokenNotificationsTest
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-3.xml:
PHPUnit Test failed to complete; Error: PHPUnit 6.5.14 by Sebastian Bergmann and contributors.
Testing Drupal\Tests\content_moderation_notifications\Kernel\TokenNotificationsTest
.F 2 / 2 (100%)
Time: 2.47 seconds, Memory: 4.00MB
There was 1 failure:
1) Drupal\Tests\content_moderation_notifications\Kernel\TokenNotificationsTest::testEmailDelivery
Failed asserting that actual size 7 matches expected size 6.
/var/www/html/modules/contrib/content_moderation_notifications/tests/src/Kernel/NotificationsTest.php:163
Huh? Oh, then I noticed:
class TokenNotificationsTest extends NotificationsTest
Whoops. That means every test / assertion added to NotificationsTest gets run twice:
- Once as NotificationsTest
- Again as TokenNotificationsTest
This wastes time / effort / DA $$ for testbot cycles, and carbon footprint.
Steps to reproduce
- Add a failing test to NotificationsTest
- Run all the Kernel tests.
- Watch it fail twice, not once.
Proposed resolution
Option A: Use the trait
We've already got a Trait for shared test code (ContentModerationNotificationTestTrait). Put stuff that should actually be shared in there. Make both existing Kernel tests extend KernelTestBase.
Option B: Create a ContentModerationNotificationKernelTestBase class
Create an abstract base class with shared stuff, including common modules, setUp() steps, etc.
Make both Kernel tests extend that.
Remaining tasks
- Decide which approach we prefer.
- Implement it.
- Verify the tests still pass.
- Make sure we don't get duplicate test results if something fails in NotificationTest.
- Reviews / refinements
- RTBC
- Commit
User interface changes
Nope.
API changes
Only to the Kernel test classes, which isn't an API.
Data model changes
Nope.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 3226605-4.optionA.patch | 3.6 KB | dww |
Issue fork content_moderation_notifications-3226605
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:
changes
Comments
Comment #2
dwwComment #3
dwwHere's option A in patch form. Not sure I love it, but it's a start. ;)
I discovered that we only care about filter_test because it creates a 'filtered_html' text format "for free" if we enable it and install its config. Only TokenNotificationsTest cares about that, so moved that as part of the cleanup (and added a comment for the next person looking at these tests).
option B might be cleaner, but then it's not clear we even need
tests/src/Kernel/ContentModerationNotificationTestTrait.phpanymore.tests/src/Functional/Form/CrudFormTest.phpuses it, but never callsenableModeration().Probably
CrudFormTestshould just useDrupal\Tests\content_moderation\Traits\ContentModerationTestTraitdirectly, we should removeContentModerationNotificationTestTrait, and have aContentModerationNotificationKernelTestBaseto share code across the Kernel test classes... 🤔Comment #4
dwwWhoops, attaching the patch would help. 🤦♂️😉
Comment #7
bkosborneI was also confused by this. Thanks for starting the work on this. I took it further in an MR and will add you to issue credits. Thanks!