Add unit tests for add-nodes workflow related to auth tokens #9218
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
pawanpinjarkar
force-pushed
the
add-unit-tests
branch
from
36dde2b
to
d0025cb
Compare
|
New changes are detected. LGTM label has been removed. |
pawanpinjarkar
force-pushed
the
add-unit-tests
branch
3 times, most recently
from
29e884d
to
9e91897
Compare
|
/test e2e-agent-compact-ipv4 |
|
Please add in the description "Requires #9210" |
| assert.Equal(t, ok, true) | ||
| if tc.workflow == workflow.AgentWorkflowTypeAddNodes { | ||
| // Retrieve the cluster secret and verify there are no errors | ||
| clusterSecret, err := fakeClient.CoreV1().Secrets(authTokenSecretNamespace).Get(context.Background(), authTokenSecretName, metav1.GetOptions{}) |
There was a problem hiding this comment.
isn't enough to reference tc.clusterSecret(t) here?
There was a problem hiding this comment.
introduces more changes to fake the time
| name: "generate-public-key-and-token-install-workflow", | ||
| workflow: workflow.AgentWorkflowTypeInstall, | ||
| name: "generate-public-key-and-token-for-install-workflow", | ||
| ocpVersion: defaultOCPVersion, |
There was a problem hiding this comment.
It seems to me quite misleading to explicitly mention the ocp version, given that the tested code doesn't use it (explicitly). The assumption it's just that in previous versions (than 4.18) some secret datas are not available - and they need to be reconciled.
In theory, this could happen even if (for some obscure reason) the user may decide to delete them. Ofc it's not a realistic code, but the current code is designed to fix this erroneous condition in any case, regardless of the current ocp version - and the test should reflect that.
There was a problem hiding this comment.
(This very likely could make the test code simpler, but I could be wrong)
There was a problem hiding this comment.
ocpVersion in reality has nothing to do but just used as a way to signify the case when there is only 1 token in the secret
There was a problem hiding this comment.
And that was somehow misleading, as I was expecting something related to the version. I'd feel that by removing it the test will become more clear
There was a problem hiding this comment.
thanks, yes removed it, instead used a field, number expected auth tokens
pawanpinjarkar
force-pushed
the
add-unit-tests
branch
from
9e91897
to
23fc664
Compare
pawanpinjarkar
force-pushed
the
add-unit-tests
branch
from
23fc664
to
314e71b
Compare
|
@pawanpinjarkar: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions 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. I understand the commands that are listed here. |
|
/test e2e-agent-compact-ipv4 |
| }) | ||
| } | ||
| } | ||
|
|
||
| func secretWithOnly1Token(expiry time.Time) func(t *testing.T) runtime.Object { |
There was a problem hiding this comment.
I think the secretWithOnly1Token and secretwith3Tokens functions are circumventing the goal of the unit tests. AuthConfig.Generate and the unit tests should be calling the same sets of functions to create the Secrets or contents of the Secrets that stores the tokens. Otherwise the unit test isn't testing the actual code.
|
/test e2e-agent-compact-ipv4-appliance-diskimage |
|
PR needs rebase. Instructions 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. |
openshift-merge-robot
added
the
needs-rebase
No description provided.