Add e2e test for different service and revision label #766
Conversation
There was a problem hiding this comment.
@shashwathi: 0 warnings.
In response to this:
Description
Add e2e test for different service and revision label
Reference
Fixes #718
/assign @rhuss
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/test-infra repository.
knative-prow-robot
added
the
size/S
test/e2e/service_options_test.go
Outdated
| templateLabelsJsonpathFormat := "jsonpath={.spec.template.metadata.labels.%s}" | ||
|
|
||
| for k, v := range serviceLabels { | ||
| out := test.kn.Run("service", "describe", serviceName, "-o", fmt.Sprintf(metadataLabelsJsonpathFormat, k)) |
There was a problem hiding this comment.
Could you not run the command once and verify output with all labels after?
There was a problem hiding this comment.
Thanks for the feedback. I have updated the PR to do the same.
knative-prow-robot
added
the
approved
test/e2e/service_options_test.go
Outdated
| gotServiceLabels := service.ObjectMeta.GetLabels() | ||
| for k, v := range serviceLabels { | ||
| assert.Equal(t, gotServiceLabels[k], v) | ||
| } |
There was a problem hiding this comment.
For a full check, you would also need to check that every label in gotServiceLabels has been verified (i.e. that the labels returned by the service are really all stored in gotServiceLabels).
Imagine that GetLabels returns an empty map (so no label has been created, which would be a bug). But your validation would still succeed.
Actually you should remove the entry in gotServiceLabels after each assert and then check after the loop that gotServiceLabels is indeed empty.
There was a problem hiding this comment.
Same btw for the loop above, too.
There was a problem hiding this comment.
Imagine that GetLabels returns an empty map (so no label has been created, which would be a bug). But your validation would still succeed.
If GetLabels returns an empty map and serviceLabels has any label to be validated then validation logic will fail.
There was a problem hiding this comment.
Sorry, you are right with GetLabel returning an empty map would lead to an error. But would happen if GetLabel returns more labels than expected ? Is this supposed to be an error is this something that is also expected ?
Of course this is a question for what to check. If you have full control over the service, I would expect that there are no extra labels being added to revisions and annotations (so would still suggest to check that all of the labels you get with GetLabel are really expected). In other words, I would check that both maps are equal not that the expected map is subset of the actual map.
There was a problem hiding this comment.
If you have full control over the service, I would expect that there are no extra labels being added to revisions and annotations
Thanks for explaining the pretext here. I see your argument now about checking for exact match. 👍
There was a problem hiding this comment.
I have updated the PR. Please review it at your convenience
knative-prow-robot
added
the
needs-rebase
knative-prow-robot
added
needs-rebase
- Store results from one call and validate both revision and service labels - Update var name for more better readability Signed-off-by: Andrew Su <[email protected]>
knative-prow-robot
removed
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maximilien, rhuss, shashwathi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test pull-knative-client-integration-tests-latest-release |
|
/retest |
1 similar comment
|
/retest |
…e#766) * Introduce -reusenamespace test flag (knative#1383) * Introduce -reusenamespace test flag * allows for reusing test namespaces that were created in advance (possibly by a cluster admininstrator) * Fix gofmt * Run tests as project admin (knative#1384) * Create Broker explicitly rather than by labeling namespace * creating explicity is allowed for users with project admin permissions in the given project * Use Role instead of ClusterRole to work with Sources * this allows for running the tests as project admin users and don't require cluster-admin permissions * Separate SourceList test for cluster admin and regular user * Instructions for running E2E as project admin * Fix imports * Define output as a new variable in source_list_crd_test * Fix golint * Reference an issue created for source list-types * Use namespaced kubectl for sa,role,rolebinding in tests (knative#1396)
Description
Add e2e test for different service and revision label
Reference
Fixes #718
/assign @rhuss