Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make log_data required in mailer.send #9416

Merged
merged 1 commit into from
Mar 26, 2025
Merged

Conversation

mtomilov
Copy link
Contributor

@mtomilov mtomilov commented Mar 21, 2025

Refs #9288
Supersedes #9414

This is a follow-up PR to #9337 we make log_data in mailer.send a required argument which was optional to support existing in-flight messages lacking it.

Testing

  • Log in as devdata_admin
  • Create a reply / mention notification of devdata_user
  • Observe Send email ... in the logs

@mtomilov mtomilov force-pushed the make-log-data-required branch from ff43783 to 7e42d66 Compare March 24, 2025 15:06
@@ -179,15 +182,20 @@ def svc(self, pyramid_request, user_password_service, subscription_service):

@pytest.fixture(autouse=True)
def tasks_mailer(self, patch):
return patch("h.services.user_signup.tasks_mailer")
mock = patch("h.services.user_signup.tasks_mailer")
mock.send.delay = create_autospec(mailer.send.run)
Copy link
Contributor Author

@mtomilov mtomilov Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

autospec doesn't affect celery task send for whatever reason, will fix that in other tests as well.
celery.app.task.Task.run is what we need to emulate which is basically __wrapped__ on a decorator.
What that means that if the service code and test code are in agreement we weren't checking if the service code calls mailer.send with a correct signature.
Also can't use patch instead because of patch target has already been mocked out error.

@mtomilov
Copy link
Contributor Author

mtomilov commented Mar 24, 2025

Why we need to specifically autospec celery task
Regular decorators with wrap properly validate the signature while celery.task one does not.

from functools import wraps
from unittest.mock import create_autospec

from h.tasks.mailer import send


def f(a, b, c):
    pass


def decorator(func):
    @wraps(func)
    def wrapper(*args, **kwargs):
        return func(*args, **kwargs)

    return wrapper


if __name__ == "__main__":
    mock_send = create_autospec(send.run)  # we expect an error but without run nothing happens
    try:
        mock_send()
    except TypeError as e:
        print("error:", e)
    
    mock_f = create_autospec(decorator(f))
    try:
        mock_f()
    except TypeError as e:
        print("error:", e)

@mtomilov mtomilov force-pushed the make-log-data-required branch from 7e42d66 to 0b5b762 Compare March 24, 2025 15:57
@mtomilov mtomilov requested a review from seanh March 24, 2025 18:16
@mtomilov mtomilov mentioned this pull request Mar 24, 2025
@mtomilov mtomilov merged commit 68c007c into main Mar 26, 2025
11 checks passed
@mtomilov mtomilov deleted the make-log-data-required branch March 26, 2025 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants