I noticed a bunch of problems with this test while reviewing #1818764: Use specific subject line in update emails.
update.status should give you $available_updates_url while update.settings should give you $update_settings_url. The mocked urlGenerator is doing it backwards, so all the assertions are wrong and don't match the real emails._update_cron_notify() is the only place sends the status_notify email.hook_mail() implementation without parameters.UpdateMailTest::testUpdateEmail() takes an $authorized bool parameter from its data provider. However, we've deprecated the entire authorize.php world, and there's no longer any difference in the emails sent by Update Status regardless of the permissions of the target user.Look at core/modules/update/tests/src/Unit/UpdateMailTest.php
Fix the test.
| Comment | File | Size | Author |
|---|
Show commandsStart 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, plain diff MR !12876
Comments
Comment #2
dwwThis is going to conflict with #1818764: Use specific subject line in update emails, let's land that, first.
Comment #3
dwwPer Slack with @xjm, expanding scope here to actually fix
UpdateMailTeston its own, before trying to add more coverage to it at #1818764: Use specific subject line in update emails.Comment #5
dwwI believe this is ready. The test is passing locally, so if the pipeline isn't green, it's due to a random fail. 😅
Comment #6
smustgrave commentedFor this one I actually did the side-by-side review as it was easier then inline.
The matrix of core and contrib hitting every combo of current, not current, not secure, secure appear to be hit.
Question do we need coverage for REVOKED and NOT_SUPPORTED?
Comment #7
dwwThanks for the review!
Good question about the other statuses. "need" is a strong word. 😂 I'm slightly worried that every possible combination of all possible status values for core vs. contrib will explode the size of this data provider. It's not like the logic in
_update_message_text()is all that complicated. Seems plausible that we don't need to test every possible code path in there. This MR is already larger than the "combined" MR that prompted splitting this off to a separate issue. 😅 I don't want to explode the scope here more than I already have.I vote we get this in ASAP as a significant improvement to what we have now (and to unblock the bugfix that prompted this). If we decide we need more cases later, we can always add them easily enough.
Thanks,
-Derek
Comment #8
smustgrave commentedWorks for me, thanks for the quick reply!
Comment #9
quietone commented@dww, thanks for fixing my mistakes and improving this test.
I reviewed this for my own benefit and did not find any problems.
Comment #10
dwwYou’re welcome and thanks for the review. Since this task is blocking a bug fix, tagging.
Comment #11
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #12
quietone commentedRebase with a simple conflict in UpdateMailTest due to the change to using an attribute for @group. Therefor, restoring RTBC
Comment #13
quietone commentedI made a mistake in the rebase and didn't properly remove an argument from the method testUpdateEmail, which the tests found. I have corrected that and tests are passing. I think it is minor, to I am leaving this at RTBC.
Comment #15
longwaveThis is a nice cleanup. Doesn't backport cleanly to 10.x, not sure it is worth the additional effort.
Committed f7bf351 and pushed to 11.x. Thanks!
Comment #18
dwwThanks @longwave! Only reason to backport this would be if the bug fix that prompted it, #1818764: Use specific subject line in update emails, is likely to be backported, too. However, as a string change, I imagine that one is unlikely to get backported very far. Let's call this fixed for now, and we can always re-open if we decide we do want to backport it. I'm happy to re-roll if needed / desired.