Add api specs for prometheus endpoints #295
Conversation
|
@miq-bot add_label providers/containers |
|
I know you're not touching that part in the file, but I don't like the fact that we have |
zeari
force-pushed
the
prometheus_tests
branch
from
6ab017c
to
9369980
Compare
|
Checked commits zeari/manageiq-api@9369980~...6454bd2 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0 |
| prometheus_alerts_connection["endpoint"] | ||
| ) | ||
| expect(provider.authentication_token(:prometheus)).to eq(token(prometheus_connection)) | ||
| expect(provider.authentication_token(:prometheus_alerts)).to eq(token(prometheus_alerts_connection)) |
There was a problem hiding this comment.
I'm not sure this tested what we wanted to test.
AFAICT it only checks parent endpoints got created correctly; we mainly want to test child manager got created (and can see the endpoint it needs).
Consider also adding test for deletion of prometheus endpoint and maybe of whole provider resulting in child manager deletion.
There was a problem hiding this comment.
@cben testing the child manager got created should probably not be done in the API tests. If I understand correctly, API tests are supposed to test the interface, not the implementation, and the child manager is definitely an implementation detail.
There was a problem hiding this comment.
I was thinking of it doesnt blow up type tests
Should the sub-manager logic go away(which i feel it might) then we will need to change these spec tests so endpoint creation is enough IMO.
There was a problem hiding this comment.
API tests are supposed to test the interface, not the implementation, and the child manager is definitely an implementation detail.
Well, I really want an integration test that implementation reacted correctly to API somewhere.
Do you see a better place/way to test it?
OK, in principle, if we have a test in our repo that directly creating parent endpoint creates the child (do we?), and test that API creates endpoints, then in theory we're covered. I have low trust for theory in this case though :-)
I was thinking of it doesnt blow up type tests
Hmm, yes, for this specific failure it was blowing up, that's enough.
Changed to |
|
@abellotti Please review |
|
I think this is fine, testing what the inbound API request is supposed to do. Not sure if there are additional provider wiring tests to do for this scenario, but that may be best in the provider repo. So, I'm 👍 here. /cc @jntullo thoughts ? |
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1535941
spec tests for ManageIQ/manageiq-providers-kubernetes#220
This wont catch the regression handled in ManageIQ/manageiq-providers-kubernetes#220 but we rely on api too much for us not to have at least some prometheus spec tests here.
@elad661 @cben @moolitayer PTAL