Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] Add more tests around the AMQP transport#27206
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
092b559 to022c47fCompare022c47f to4b8a655Compare| publicfunctioncreateTransport(string$dsn,array$options):TransportInterface | ||
| { | ||
| returnnewAmqpTransport($this->encoder,$this->decoder,$dsn,$options,$this->debug); | ||
| returnnewAmqpTransport( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
shouldn't it be in one line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yes, it should
| newAmqpTransport($encoder,$decoder, Connection::fromDsn('amqp://localhost',array('foo' =>'bar'),true),array('foo' =>'bar'),true), | ||
| $transport | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be on one line
| $connection, | ||
| $options, | ||
| $debug | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
one line as well
| private$decoder; | ||
| private$dsn; | ||
| private$connection; | ||
| private$options; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$options and $debug are not used anymore
4b8a655 to1c0bb0eComparesroze commentedMay 9, 2018
@fabpot@yceruto@ogizanagi good catches, updated. |
| useSymfony\Component\Messenger\Transport\Serialization\EncoderInterface; | ||
| useSymfony\Component\Messenger\Transport\TransportInterface; | ||
| class AmqpTransportTestextends TestCase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should requires amqp extension
1c0bb0e tofaf9382Compare| $this->dsn =$dsn; | ||
| $this->options =$options; | ||
| $this->debug =$debug; | ||
| $this->connection =$connection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This removes laziness. I'd suggest to try harder keeping feature. The factory could validate the dsn itself if that's the goal here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Non. The laziness is alreadywithin theConnection object. There is no point of making a class creation lazy 😄 (and even that is already lazy as behind service locators)
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Got it :)
…oze)This PR was merged into the 4.1 branch.Discussion----------[Messenger] Add more tests around the AMQP transport| Q | A| ------------- | ---| Branch? | 4.1| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | ø| License | MIT| Doc PR | øAdding more tests to the AMQP transport/factory. These should have captured the following 3 bugs:#27198,#27197,#27196.Commits-------faf9382 Add more tests around the AMQP transport
Uh oh!
There was an error while loading.Please reload this page.
Adding more tests to the AMQP transport/factory. These should have captured the following 3 bugs:#27198,#27197,#27196.