Improve coverage of E2E tests #253
Conversation
|
Hi @alan-ghelardi. Thanks for your PR. I'm waiting for a tektoncd 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/test-infra repository. |
|
/ok-to-test |
3fce144 to
314be46
Compare
adambkaplan
left a comment
There was a problem hiding this comment.
Thank you so much for this PR! This will definitely help get the ball rolling on having sustainable e2e tests. And ++1 for catching a bug in the process!
I have a few suggestions about the rbac.yaml that is deployed, and a question about the kind configuration yaml. The other suggestions regarding configuring the cert pair locations are optional - IMO those should not block merge and can be captured as a follow-up item.
| kubectl kustomize "${ROOT}/test/e2e/kustomize" | ko apply -f - | ||
|
|
||
| echo "Fetching access tokens..." | ||
| tokens_dir=/tmp/tekton-results/tokens |
There was a problem hiding this comment.
Ideally this can be a configuration option for the script. This can be addressed in a follow-up PR.
| -keyout "/tmp/tekton-results-key.pem" \ | ||
| -out "/tmp/tekton-results-cert.pem" \ |
There was a problem hiding this comment.
Future TODO - make the location of the keypair configurable.
| -keyout "/tmp/tekton-results-key.pem" \ | ||
| -out "/tmp/tekton-results-cert.pem" \ |
There was a problem hiding this comment.
Ditto - as a follow up, we should make this configurable.
| ) | ||
|
|
||
| const ( | ||
| certFile = "/tmp/tekton-results-cert.pem" |
There was a problem hiding this comment.
If this path is made configurable, we would likely want to convert these constants to vars that live near a test main function.
| allNamespacesReadAccess = "/tmp/tekton-results/tokens/all-namespaces-read-access" | ||
| singleNamespaceReadAccess = "/tmp/tekton-results/tokens/single-namespace-read-access" |
There was a problem hiding this comment.
Likewise, when this is made configurable, we should move these constants near a test main.
Co-authored-by: Adam Kaplan <adam.kaplan@redhat.com>
|
/retest |
714e1fd to
1d76e6f
Compare
|
@adambkaplan thank you for the review. I've accepted your suggestions and will make the tokens path configurable in a future pr. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan 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 |
|
/assign dibyom |
|
/lgtm (Opened an issue to capture the TODOs around making the this a bit more configurable) |
khrm
left a comment
There was a problem hiding this comment.
/kind cleanup
We need this label for release as discussed in the last WG call.
Resolves #142.
What
Enhance the support for reading Results and Records across multiple collections #249. In addition, basic listing
operations were covered.
passed to the
Checkmethod was incorrect and if one created a Role to restrictthe access to a single namespace, the API server would always return an
unauthenticated error. The fix is covered in a newlycreated E2E test case.
versions of Kind and the tests will run on a cluster that uses a known
Kubernetes version. Note that Knative no longer supports the Kubernetes version
used by default in the Kind v0.10 (which is mentioned in the E2E documentation).
Additional information
There are two already existing E2E test cases failing consistently. I'm going to
open a new pull request to fix them to make changes more granular.