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

[NFC] Bring back deleted activity email tests #23577

Merged
merged 1 commit into from
May 26, 2022

Conversation

demeritcowboy
Copy link
Contributor

Overview

These tests were deleted in #23571 but they should have been migrated back when the function was deprecated.

The testSendEmailWithMultipleToRecipients may or may not still apply so have left out for the moment. Would need to do more research.

@civibot
Copy link

civibot bot commented May 25, 2022

(Standard links)

@civibot civibot bot added the master label May 25, 2022
@eileenmcnaughton
Copy link
Contributor

@demeritcowboy - so this makes the tests test the code we actually use instead of the deprecated code we no longer use? Sounds like a win

@eileenmcnaughton eileenmcnaughton merged commit d409829 into civicrm:master May 26, 2022
@demeritcowboy
Copy link
Contributor Author

I get what you're saying - there's a longstanding debate in the computer science world about whether to test internal code. I myself have mixed feelings about it. I'll note \Civi\Test\Invasive exists for this reason. Of course the answer is: "sometimes".

But I think it's not crazy to have a rule that when coverage already exists, then if the covered code blocks are moved then whatever it is those tests are covering should stay covered. I admit I don't know of a tool to help with that, so I don't know how to automate enforcing such a rule. It seems like a loophole in unit-testing.

@demeritcowboy demeritcowboy deleted the deleted-tests branch May 26, 2022 03:26
@eileenmcnaughton
Copy link
Contributor

@demeritcowboy this PR creates more testing - that's a good thing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants