Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Porting redis instrumentation from contrib repo #595
Porting redis instrumentation from contrib repo #595
Changes from 20 commits
f91ce7c
88ea6f1
8799497
4157cbf
1e48c7d
d0af1ec
6809d9a
64e1d72
2e71d4f
a98fb47
43ae8b0
8933c0b
9b7e311
5e8172d
a33287b
5f40067
2bcadf5
65c6cb3
3a1d1dd
5fd98fe
ffab5f9
039ad13
75a9bf4
28e6d7c
9913c1c
1b978a5
5d74317
d843cad
e064dc1
2712732
8907032
5ee71de
2faca48
f372d1e
87eceb6
a10916b
0757069
e4d9084
b4d810e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I'm wondering if it is a really good approach to keep the tests of the different frameworks under the same test. What about if I only want to test redis?
Is there a technical limitation to do that?
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.
there's no technical limitation. it's not the path currently but we can split them if it makes more sense (i suspect as we have more integration tests, we will need to do this to run tests in parallel)
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.
Besides parallel tests, there is also the case when you only want to run a specific test without downloading and starting the other containers that you don't need.
This could be a little bit complicated to set up, so we can ignore and handle in a follow up PR.
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.
You want to call
instrument
/uninstrument
once per test instead of once for the class?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.
Yeah, this way its not instrumenting the call to flushall which resets the redis data after each test.
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.
is it worth refactoring some of these assertions into a utility method? there's a lot of identical code with regards to db.instance / url.
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.
good call, refactored.