Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Notifier] [DX] Abstract test cases#39495

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
derrabus merged 1 commit intosymfony:5.1fromOskarStark:abstract-test-case
Dec 22, 2020

Conversation

@OskarStark
Copy link
Contributor

@OskarStarkOskarStark commentedDec 14, 2020•
edited
Loading

QA
Branch?5.1
New feature?no
Deprecations?no
Tickets---
LicenseMIT
Doc PR---

This PR

  • adds a newabstractTransportTestCase
  • adds a newabstractTransportFactoryTestCase (code is mainly taken from theMailer/TransportFactoryTestCase)

We have a lot of code duplication in the notifier bridges

Todos

  • check if we want this
  • I would want to use Dsn strings (like already used in the notifier bridge tests) instead of objects for the providers, what do you think? For me it is more readably
  • update all bridges
  • Bump notifier to~5.1.10

Questions

  • is it Ok to consider this a bugfix and merge into5.1?
  • shall I prefix the abstract test cases withAbstract ? As we did the same for Mailer, I would say no

@symfony/mergers have to change ^5.2 into ^5.2.1

Koc reacted with eyes emoji
@carsonbotcarsonbot added Status: Needs Review Notifier DXDX = Developer eXperience (anything that improves the experience of using Symfony) labelsDec 14, 2020
@carsonbotcarsonbot added this to the5.1 milestoneDec 14, 2020
@OskarStarkOskarStarkforce-pushed theabstract-test-case branch 2 times, most recently from59dc394 to2996190CompareDecember 14, 2020 14:53
fabpot added a commit that referenced this pull requestDec 15, 2020
… (OskarStark)This PR was merged into the 5.1 branch.Discussion----------[Notifier]  [Free Mobile] Could not use custom host in DSN| Q             | A| ------------- | ---| Branch?       | 5.1| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       | ---| License       | MIT| Doc PR        | ---While working on#39509 I discovered, that you cannot set a custom host through the DSN string itself, only by calling `setHost()` method in the transport, which is only possible by **not** using the factory....I changed it the way all other bridges work. I don't add a testcase for the port, because non of the others have that test.I plan to implement it in#39495As this is a bugfix I created an extra PR.CheersEDIT:Also the host is not allowed to contain `https://` otherwise calling `__toString()` will result in: `freemobile://https://......`Commits-------63350cc [Notifier] [Free Mobile] Could not use custom host in DSN
@OskarStarkOskarStark changed the title[Notifier] [DX] Use abstract factory test case[Notifier] [DX] Use abstract test caseDec 16, 2020
@OskarStarkOskarStark changed the title[Notifier] [DX] Use abstract test case[Notifier] [DX] Use abstract test casesDec 16, 2020
@OskarStarkOskarStarkforce-pushed theabstract-test-case branch 4 times, most recently froma030df5 to8d7700fCompareDecember 18, 2020 11:09
@OskarStarkOskarStark changed the title[Notifier] [DX] Use abstract test cases[Notifier] [DX] Abstract test casesDec 19, 2020
"php":">=7.2.5",
"symfony/http-client":"^4.3|^5.0",
"symfony/notifier":"~5.1.0"
"symfony/notifier":"~5.1.10"
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I guess I need to bump this to ~5.1.11, now that 5.1.10 was released, right?

@OskarStark
Copy link
ContributorAuthor

Ready to merge from my side 👍

Copy link
Member

@derrabusderrabus left a comment

Choose a reason for hiding this comment

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

Okay, first of all, I really like the new test case classes. They give the tests of all bridges more structure and makes it easier to maintain.

But:

  • We're basically rewriting the test suites of all notifier bridges for the 5.1 branch here. That branch is going to die next month.
  • I have merged several tests refactorings from 5.1 to 5.2 and had to solve quite a few conflicts each time. Slack bridge, I'm looking at you.
  • Quite a few bridges have changed fundamentally in 5.2. It is close to impossible to merge this PR cleanly to 5.2 and beyond.

So, this PR certainly improves things, but I wonder if we should really target 5.1.

@OskarStark
Copy link
ContributorAuthor

OskarStark commentedDec 22, 2020•
edited
Loading

So, this PR certainly improves things, but I wonder if we should really target 5.1.

If you like you can just merge it in5.1 and keep the5.2 like it is. I would then create a follow up PR against5.2, so no need to fix all the merge conflicts. But the only thing (but not complicated at all) is Slack.I tried to merge5.2 in my branch and all merge conflicts are not hard to fix, because of my PR#39428, which I created before 👍

@derrabus
Copy link
Member

Thank you Oskar.

@derrabusderrabus merged commit1ee1659 intosymfony:5.1Dec 22, 2020
@OskarStarkOskarStark deleted the abstract-test-case branchDecember 22, 2020 14:17
nicolas-grekas added a commit that referenced this pull requestJan 7, 2021
This PR was squashed before being merged into the 5.2 branch.Discussion----------[Notifier] Use abstract test cases in 5.2| Q             | A| ------------- | ---| Branch?       | 5.2| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | Follows#39495| License       | MIT| Doc PR        | ---Same as#39495, but for `5.2`cc@derrabusCommits-------8f6b08c [Notifier] Use abstract test cases in 5.2
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@jderussejderussejderusse approved these changes

@derrabusderrabusderrabus approved these changes

@nicolas-grekasnicolas-grekasAwaiting requested review from nicolas-grekas

@wouterjwouterjAwaiting requested review from wouterj

Assignees

No one assigned

Labels

DXDX = Developer eXperience (anything that improves the experience of using Symfony)NotifierStatus: Reviewed

Projects

None yet

Milestone

5.1

Development

Successfully merging this pull request may close these issues.

6 participants

@OskarStark@derrabus@nicolas-grekas@jderusse@wouterj@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp