Skip to content

[Mailer] Move abstract test case to Tests/ #39530

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

[Mailer] Move abstract test case to Tests/ #39530

wants to merge 6 commits into from

Conversation

OskarStark
Copy link
Contributor

Q A
Branch? 4.4
Bug fix? no
New feature? no
Deprecations? no
Tickets ---
License MIT
Doc PR ---

This PR

is based on https://github.com/symfony/symfony/pull/39495/files#r544203249

cc @wouterj @nicolas-grekas

@carsonbot carsonbot added this to the 4.4 milestone Dec 17, 2020
@OskarStark OskarStark changed the title [Mailer] Move abstract test caste to Tests/ [Mailer] Move abstract test case to Tests/ Dec 17, 2020
@OskarStark OskarStark mentioned this pull request Dec 17, 2020
Merged
8 tasks
nicolas-grekas
nicolas-grekas reviewed Dec 17, 2020
View reviewed changes
@OskarStark
Copy link
Contributor Author

@nicolas-grekas I bumped all in require section

@xabbuh
Copy link
Member

xabbuh commented Dec 17, 2020

That's a BC break, isn't it?

nicolas-grekas
nicolas-grekas approved these changes Dec 17, 2020
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

That's a BC break, isn't it?

in theory yes, in practice using this Test namespace was just a workaround for something that is now fixed by the new line in .github/composer-config.json

@wouterj
Copy link
Member

wouterj commented Dec 17, 2020

I'm 👎 here. We have quite some components offering these abstract test classes to be used outside the Symfony framework. They make testing custom mailer transports easier for everyone. This even was the reason this class was moved in Test (from Tests) in #32749

In any case, if this is still approved it is a BC break and can't be done in 4.4 imho. But I believe we should keep this as-is and consider the abstract Notifier test case (related PR) a feature.

@nicolas-grekas
Copy link
Member

Thanks for the context @wouterj this makes perfect sense!
I'm OK then for doing the same in Notifier!

@wouterj
Copy link
Member

wouterj commented Dec 17, 2020

(I'm sorry if my comment was a bit blunt. With one approval in already, I was in a bit of a rush to prevent quick merge)

@OskarStark
Copy link
Contributor Author

(I'm sorry if my comment was a bit blunt. With one approval in already, I was in a bit of a rush to prevent quick merge)

All fine , thanks 👍

@OskarStark OskarStark deleted the move-test-cases branch December 17, 2020 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Mailer Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
5 participants
@OskarStark @xabbuh @wouterj @nicolas-grekas @carsonbot