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

Update the example app to work with latest Broadway and Broadway SQS #56

Merged
merged 9 commits into from Jan 13, 2021

Conversation

philss
Copy link
Member

@philss philss commented Jan 13, 2021

Also add the example app to our CI pipeline.

@josevalim
Copy link
Member

I think we can also clean up and remove the comments of the config files, WDYT?

@josevalim
Copy link
Member

Something else we could do is to change the example app to use Broadway.DummyProducer. This way, you no longer need to define a TestClient that has an empty receive_message. WDYT?

@philss
Copy link
Member Author

philss commented Jan 13, 2021

Something else we could do is to change the example app to use Broadway.DummyProducer. This way, you no longer need to define a TestClient that has an empty receive_message. WDYT?

I think the intention of the TestClient is to demonstrate that the BroadwaySQS.Producer is working good if you define a custom SQSClient. I'm not sure if replacing with the DummyProducer will be beneficial here because if we do it won't be testing anything related to the SQS producer. WDYT?

@josevalim
Copy link
Member

Tge example app is meant to be an example for other devs, no? if so, best practice today is the dummy producer. The functionality is tested on sqs itself (if not, it should!).

This is because we don't need to really interact with the SQS
implementation in the tests.
@philss
Copy link
Member Author

philss commented Jan 13, 2021

@josevalim yes, makes sense! I changed to use Broadway.DummyProducer 👍

@philss philss merged commit 73e75d2 into master Jan 13, 2021
@philss philss deleted the ps-update-example-app branch January 13, 2021 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants