Skip to content
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

Minimal test suite for kadmin.local #346

Closed
wants to merge 2 commits into from

Conversation

martbab
Copy link
Contributor

@martbab martbab commented Dec 15, 2016

This ensures that we do not break it (much) in the
future like we did recently

https://fedorahosted.org/freeipa/ticket/6561

Martin Babinsky added 2 commits December 15, 2016 17:40
This allows for diagnose the output and error code of these operations.
Otherwise there is no way to infer their success or failure apart from
inspecting logs post-mortem.

https://fedorahosted.org/freeipa/ticket/6561
This small integration suite tests some basic operations using
kadmin.local interface on services in both kerberos and services
subtree.

https://fedorahosted.org/freeipa/ticket/6561
to catch the command's stderr and check that it is empty
"""
result = command(*args)
assert not result.error_output
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldnt be here tested rather return code than stderr?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the last case, there was a Kerberos internal database error but the return code was still 0 (also pointed out in the comment). I wonder, could this cause issues should Kerberos throw some warnings on stderr or do we assume no warnings should be present at all at any time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warnings are also thrown in stderr (e.g. when no password policy was specified) but they are not tested in this case as they are part of setup. The problem here is, as Standa pointed out, that if the query fails, kardmin still returns 0, so testing for the presence of stderr is still the sanest way to assert command success. But I can re-implement the test to assert success by expecting certain messages in stdout if you wish.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, OK blind me a didn't see the commet, ACK

@MartinBasti MartinBasti added the ack Pull Request approved, can be merged label Dec 16, 2016
@martbab martbab added the pushed Pull Request has already been pushed label Dec 16, 2016
@martbab martbab closed this Dec 16, 2016
@martbab martbab deleted the kadmin-test branch December 16, 2016 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
3 participants