Conformance: Improve test reliability#353
Conversation
|
Welcome @mazdakn! |
|
Hi @mazdakn. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
✅ Deploy Preview for kubernetes-sigs-network-policy-api ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
4524c64 to
8137394
Compare
|
/ok-to-test |
|
@mazdakn this is a very large PR and would be much easier to review if it was broken up into multiple smaller commits for the different pieces... |
|
@danwinship this PR looks like a large PR but honestly the main changes are in the 2 files under
The tests are only changed to:
I'll try to break this PR into smaller commits, but I'm afraid the result would not be much different. Most likely it would be 2 commits each with roughly half of total LOC changes. This is due to the fact that changes to utility functions needs to be applied to all test files. |
npinaeva
left a comment
There was a problem hiding this comment.
Some nits, otherwise nice change!
| Namespace: namespace, | ||
| Name: name, | ||
| }, pod) | ||
| require.NoErrorf(t, err, "unable to fetch %s/%s", namespace, name) |
There was a problem hiding this comment.
should we say "unable to fetch pod ..."?
| } | ||
|
|
||
| // PokeServer is a utility function that checks if the connection from the provided clientPod in clientNamespace towards the targetHost:targetPort | ||
| func PokeServer(t *testing.T, client k8sclient.Interface, kubeConfig *rest.Config, clientNamespace, clientPod, protocol, targetHost string, targetPort int32, timeoutConfig config.TimeoutConfig, shouldConnect bool) { |
There was a problem hiding this comment.
I think this could use a comment. We should explain that this function waits for the expected connection result, and then checks it once more to make sure the original result wasn't an accident. I think retrying once is a good balance for now between making sure the new condition is stable and keeping the total test time reasonable, but we may want to increase it in the future
There was a problem hiding this comment.
Yes, make sense. Add some comments.
I think retrying once is a good balance for now between making sure the new condition is stable and keeping the total test time reasonable, but we may want to increase it in the future
Exactly. I was tempted to assert connectivity more than once, but I thought it can be done later if needed.
|
@npinaeva thanks for the review. Addressed your comments. PTAL. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mazdakn, npinaeva The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ClusterNetworkPolicy conformance tests are flaky due to two issues:
This PR improve the quality of conformance tests, and removes sources of flakiness. The proposed changes include:
podandcnpresources with helper functions, which helps with test readability.PokeServerutility function, as the desired states are expected to happen eventually. This seems to be a known issue and mentioned here: EnhancepokeServerin helper utilities #108In the Calico implementation of ClusterNetworkPolicy, by applying these changes conformance tests passes consistently.