-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add integration tests #109
Conversation
7907c13
to
02229d9
Compare
Not only the write path was broken due to interface changes, the read path also did not work.
02229d9
to
7d34da2
Compare
9c5be73
to
6b26e53
Compare
6b26e53
to
c87665d
Compare
prometheus-api-client<0.6 | ||
pytest<8 | ||
pytest-env<2 | ||
requests<3 |
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 we also add a corresponding section to the dependabot config, to keep the test requirements up to date?
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.
That was clearly an oversight, thank you so much for spotting that flaw. Fixed with f43b3d4.
|
||
Run integration tests:: | ||
|
||
pytest |
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 mention to run pip3 install --upgrade -r requirements-test.txt
before?
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.
Absolutely, thank you again. I've expanded this a bit more to also educate readers about how to set up the Python sandbox (virtualenv). See 87f8e11.
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.
May I ask you to review both spots once more and acknowledge the PR again if you agree with the updates? Thanks!
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.
Looks good, thanks!
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.
Beautiful
Thanks for the reviews. Merging the dependency update patches enumerated above feels so confident now -- good that we finally gained such a test layer for this piece of software. |
About
As we learned through GH-88, the unit tests are not sufficient to validate the code base thoroughly. This patch intends to bring in integration tests with real instances of Prometheus, CrateDB, and CrateDB Prometheus Adapter, effectively adding end-to-end test cases, so that corresponding required adjustments/fixes like GH-96 or 2043818 will not be missed in the future.
Details
The patch also includes the commit 2043818, which is actual a bugfix accompanying GH-96. It has been discovered while adding the test cases validating the read path.
Screenshot
Outlook
The integration tests added on behalf of this patch will ensure that bringing in those upcoming dependency update patches will not break the application again.
/cc @matriv, @seut, @ckurze, @wierdvanderhaar