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.

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

Redis Integration Tests #543

Merged
merged 1 commit into from
Jun 9, 2022
Merged

Redis Integration Tests #543

merged 1 commit into from
Jun 9, 2022

Conversation

ecumene
Copy link
Contributor

@ecumene ecumene commented Jun 8, 2022 •
edited
Loading

closes #179

@lann
Copy link
Collaborator

lann commented Jun 8, 2022

@fibonacci1729

lann
lann reviewed Jun 8, 2022
View reviewed changes
Copy link
Collaborator

@lann lann left a comment

Choose a reason for hiding this comment

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

One nit to make the linter happy, but otherwise this seems like a clear improvement. 👍

crates/redis/src/tests.rs Outdated Show resolved Hide resolved
lann
lann reviewed Jun 8, 2022
View reviewed changes
crates/redis/src/tests.rs Outdated Show resolved Hide resolved
fibonacci1729
fibonacci1729 approved these changes Jun 8, 2022
View reviewed changes
Copy link
Contributor

@fibonacci1729 fibonacci1729 left a comment

Choose a reason for hiding this comment

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

Thank you @ecumene! This is great! Once @lann's comments are addressed im ok with merging this!

@ecumene
Copy link
Contributor Author

ecumene commented Jun 8, 2022 •
edited
Loading

Sweet, but is it enough for testing the trigger (i.e closes #179)? Or should I handle some other cases?

I'll get on this after work :)

@lann
Copy link
Collaborator

lann commented Jun 8, 2022

I think ideally we would also check that the message echoed to stdout matches the input, but that would require more structural changes to the way stdio is managed in the engine.

@ecumene ecumene marked this pull request as ready for review June 9, 2022 01:22
@ecumene
Copy link
Contributor Author

ecumene commented Jun 9, 2022

Note: I added closes #179, if that's okay with you guys.

@ecumene
Fixed Redis trigger integration tests
25d1daf 
Signed-off-by: Mitchell Hynes <ecumene@users.noreply.github.com>
@ecumene
Copy link
Contributor Author

ecumene commented Jun 9, 2022

Added the changes for the Makefile to include integration tests. If it doesn't pass CI, that's why!

@fibonacci1729
Copy link
Contributor

Once CI finishes, I'll merge! Thanks again!!

@fibonacci1729 fibonacci1729 merged commit db9cffe into fermyon:main Jun 9, 2022
@ecumene ecumene deleted the redis-integration-tests branch June 9, 2022 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lann lann lann left review comments

@fibonacci1729 fibonacci1729 fibonacci1729 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add tests for the Redis trigger
3 participants
@ecumene @lann @fibonacci1729