-
Notifications
You must be signed in to change notification settings - Fork 13
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 test for invalid credentials in dns provider #520
Conversation
@averevki this test looks good but I failed to make it pass on kua-stage-single today. First the DNSPolicy message was So I made sure that HTTPRoute is also created as part of the test. Then the record condition looked like:
So I think that However, I was not able to verify this because I was unable to get to that point again for some reason. No idea why. Both Gateway and DNSPolicy were updated constantly by operators and DNSPolicy was saying that: |
Also, when running the test I noticed that the GW reporting errors related to TLS setup - TLS policy is not committed at all. Not sure if that makes a difference, I fixed this too but it did not help me make this test pass. |
I was not able to reproduce this today. Strange. But my other two comments seem to be valid:
|
testsuite/tests/singlecluster/gateway/dnspolicy/test_invalid_credentials.py
Show resolved
Hide resolved
testsuite/tests/singlecluster/gateway/dnspolicy/test_invalid_credentials.py
Outdated
Show resolved
Hide resolved
Slightly unrelated but can you think of an easy way to implement so that there is clear what the DNSPolicy status was and what the expected status is when the last assertion fails? I can only see what is expected but not really the actual status. |
Do you mean to check for successful enforcement first and then provoke the error status? Or just add comments to the assertions? |
What I meant was that the test failed for me on that last assertion and it was not clear from the test log what the actual dnspolicy status was. It would be nice if we could see both what the expected status of dns policy and actual status of dns policy were. |
2ae8c75
to
5755d9d
Compare
@trepel assert message should be clearer now. Although I see we have this problem in more places where it can be improved later |
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.
LGTM now. Not sure how stable this test will be given the constant DNSPolicy CR status updates but there is no way of knowing beforehand. It passed for me, also the assertion output in case of failure is much more informative. I think this is good to go, we can skip it if it fails in nightlies too frequently.
Constant status updates should be fixed now 👍 (Kuadrant/dns-operator#218) |
Signed-off-by: averevki <[email protected]>
5755d9d
to
7923a61
Compare
rebase^ |
Closes #519