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
feat: Add RedisTimeSeries plugin #11054
feat: Add RedisTimeSeries plugin #11054
Changes from 18 commits
ba8803c
21ff22a
27c2e74
e2317db
9feb1d6
ed46aba
03c2676
1f50c47
2503242
ff61ece
557a6ca
3f3aa27
61d6c45
1452de8
cffe9ad
97de1e2
eaebdd5
0d5c540
90653df
4fde7a0
2f98527
e12d4d8
3f57e14
ed0f52d
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.
As there is zero testing in this PR that would get routinely run, I'd like to see an integration test.
There is an example in the redis input using testcontainers that you can copy and paste from.
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.
@powersj Maybe I misunderstand the testing (and likely)! But doesn't the TestConnectAndWrite in this file, with the MockMetrics cover this use case? I'd like to make sure I'm not missing something - and I'm pretty sure I am.
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.
The point is that you have only a single test and this test isn't run by default (see
if testing.Short() { ...Skip...}
) as it requires a local instance ofredis
running. @powersj added some infrastructure to enable integration tests usingtestcontainers
(aka docker images) to verify functionality of plugins.So the request is to make use of this infrastructure (see the plugin linked above) to at least enable integration tests for your plugin...
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.
To allow integration tests you want something like this