Skip to content

[Notifier] Use abstract test cases in 5.x #39749

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

Merged
merged 1 commit into from
Jan 9, 2021

Conversation

OskarStark
Copy link
Contributor

@OskarStark OskarStark commented Jan 7, 2021

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

Same as #39736, but for 5.x

cc @derrabus

@nicolas-grekas
Copy link
Member

I did not merge your changes from #39736 into 5.x because I didn't know how to resolve merge conflicts.

Could you please update this PR to include the relevant changes that should be borrowed from 5.2?

@OskarStark
Copy link
Contributor Author

Sure 👍

@OskarStark OskarStark force-pushed the abstract-testcase-5.x branch 4 times, most recently from c4138b4 to 3a94745 Compare January 7, 2021 17:28
@OskarStark
Copy link
Contributor Author

OskarStark commented Jan 8, 2021

Looks ok now, don't know why two bridges have:

Fatal error: Uncaught Error: Class 'Symfony\Component\Notifier\Tests\TransportFactoryTestCase' not found in /home/travis/build/symfony/symfony/src/Symfony/Component/Notifier/Bridge/Discord/Tests/DiscordTransportFactoryTest.php:18
Fatal error: Uncaught Error: Class 'Symfony\Component\Notifier\Tests\TransportFactoryTestCase' not found in /home/travis/build/symfony/symfony/src/Symfony/Component/Notifier/Bridge/Esendex/Tests/EsendexTransportFactoryTest.php:18

but not the other 🤔

cc @derrabus @nicolas-grekas

@derrabus
Copy link
Member

derrabus commented Jan 8, 2021

Didn't we want to move those abstract test classes out of the Tests folder anyway on 5.x?

@OskarStark
Copy link
Contributor Author

Let's do it a little bit later because IIRC some open PRs for new bridges use it like this, and I want to get them merged before, otherwise it's a never ending story for them to get in 😑

@derrabus derrabus force-pushed the abstract-testcase-5.x branch from 3a94745 to c233636 Compare January 9, 2021 22:55
@derrabus
Copy link
Member

derrabus commented Jan 9, 2021

Those failures look like composer hickups to me: Composer could not fetch from git so it fell back to fetching the ZIP. I've rebased the PR to verify my hypothesis.

@derrabus
Copy link
Member

derrabus commented Jan 9, 2021

Thank you Oskar.

@derrabus derrabus merged commit 76f02fd into symfony:5.x Jan 9, 2021
@OskarStark
Copy link
Contributor Author

Thanks for the feedback ✌🏻

@OskarStark OskarStark deleted the abstract-testcase-5.x branch January 10, 2021 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants