-
-
Notifications
You must be signed in to change notification settings - Fork 232
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
test: add envtest for redis standalone #748
test: add envtest for redis standalone #748
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #748 +/- ##
==========================================
+ Coverage 29.25% 29.89% +0.64%
==========================================
Files 19 19
Lines 3241 3241
==========================================
+ Hits 948 969 +21
+ Misses 2258 2233 -25
- Partials 35 39 +4 ☔ View full report in Codecov by Sentry. |
be17e6a
to
2375205
Compare
Actually, we use chainsaw to do some e2e test, plz see https://github.com/OT-CONTAINER-KIT/redis-operator/tree/master/tests/e2e-chainsaw/v1beta2. :) |
So you think there is no point in doing integration test ? e2e is enough ? I like integration tests because it's fast to run while still testing the reconcile logic in an holistic way. |
Integration tests is nice of course. |
Thanks for your input. I asked because I would like to know if I should start the feature or if it's not gonna be accepted anyway. |
IMO, it's a nice feature for the operator. |
Integration testing is great you can add that👌 |
7579ca5
to
158cca9
Compare
13905ce
to
79c4fb7
Compare
Signed-off-by: Mathieu Cesbron <[email protected]>
79c4fb7
to
0935e54
Compare
I have added the setup for integration test and integration tests for redis standalone. We check that statefulset, service,... are created and that deleting the CR delete the statefulset. I will add integration tests for the other redis (cluster, replication, sentinel) each in his own PR if you don't mind. The code coverage is now 29.89% 🥳 |
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.
nice work!
Signed-off-by: Mathieu Cesbron <[email protected]> Signed-off-by: Matt Robinson <[email protected]>
Description
I feel like it would be a good idea to add integration tests with EnvTest on the reconcile loop like mentioned here.
Would this feature be accepted if I make it ?
Edit: Feature accepted
Fixes #656
Type of change
Checklist