Improve coverage of E2E tests #253

alan-ghelardi · 2022-10-13T13:37:57Z

Resolves #142.

What

Write E2E test cases for the changes introduced on
Enhance the support for reading Results and Records across multiple collections #249. In addition, basic listing
operations were covered.
Fix a bug in the authorization flow for the ListRecords operation. The parent
passed to the Check method was incorrect and if one created a Role to restrict
the access to a single namespace, the API server would always return an
unauthenticated error. The fix is covered in a newlycreated E2E test case.
Pin Kubernetes v1.22 in the Kind config. By doing so, users may run newer
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.

tekton-robot · 2022-10-13T13:38:07Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

vdemeester · 2022-10-13T13:44:59Z

/ok-to-test

adambkaplan

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.

test/e2e/kustomize/rbac.yaml

test/e2e/kind-cluster.yaml

adambkaplan · 2022-10-13T16:36:30Z

test/e2e/01-install.sh

 set -e
 kubectl create secret tls -n tekton-pipelines tekton-results-tls --cert="/tmp/tekton-results-cert.pem" --key="/tmp/tekton-results-key.pem" || true

 echo "Installing Tekton Results..."
 kubectl kustomize "${ROOT}/test/e2e/kustomize" | ko apply -f -

+echo "Fetching access tokens..."
+tokens_dir=/tmp/tekton-results/tokens


Ideally this can be a configuration option for the script. This can be addressed in a follow-up PR.

adambkaplan · 2022-10-13T16:37:55Z

test/e2e/01-install.sh

+        -keyout "/tmp/tekton-results-key.pem" \
+        -out "/tmp/tekton-results-cert.pem" \


Future TODO - make the location of the keypair configurable.

adambkaplan · 2022-10-13T16:38:22Z

test/e2e/01-install.sh

+            -keyout "/tmp/tekton-results-key.pem" \
+            -out "/tmp/tekton-results-cert.pem" \


Ditto - as a follow up, we should make this configurable.

adambkaplan · 2022-10-13T16:40:02Z

test/e2e/grpc_client.go

+)
+
+const (
+	certFile                  = "/tmp/tekton-results-cert.pem"


If this path is made configurable, we would likely want to convert these constants to vars that live near a test main function.

adambkaplan · 2022-10-13T16:40:28Z

test/e2e/grpc_client.go

+	allNamespacesReadAccess   = "/tmp/tekton-results/tokens/all-namespaces-read-access"
+	singleNamespaceReadAccess = "/tmp/tekton-results/tokens/single-namespace-read-access"


Likewise, when this is made configurable, we should move these constants near a test main.

Co-authored-by: Adam Kaplan <[email protected]>

alan-ghelardi · 2022-10-13T17:59:58Z

/retest

alan-ghelardi · 2022-10-13T18:11:10Z

@adambkaplan thank you for the review. I've accepted your suggestions and will make the tokens path configurable in a future pr.

adambkaplan

/approve

tekton-robot · 2022-10-13T22:56:00Z

[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

Needs approval from an approver in each of these files:

~~OWNERS~~ [adambkaplan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

alan-ghelardi · 2022-10-17T12:44:11Z

/assign dibyom

dibyom · 2022-10-18T14:42:02Z

/lgtm

(Opened an issue to capture the TODOs around making the this a bit more configurable)

khrm

/kind cleanup
We need this label for release as discussed in the last WG call.

tekton-robot requested review from dibyom, imjasonh and vdemeester October 13, 2022 13:38

tekton-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 13, 2022

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 13, 2022

Write e2e test cases for listing operations.

314be46

alan-ghelardi force-pushed the improve-e2e-tests branch from 3fce144 to 314be46 Compare October 13, 2022 15:02

adambkaplan reviewed Oct 13, 2022

View reviewed changes

Apply suggestions from code review

6168228

Co-authored-by: Adam Kaplan <[email protected]>

Add a few comments to the kind-cluster.yaml file.

1d76e6f

alan-ghelardi force-pushed the improve-e2e-tests branch from 714e1fd to 1d76e6f Compare October 13, 2022 18:04

adambkaplan approved these changes Oct 13, 2022

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 13, 2022

tekton-robot assigned dibyom Oct 17, 2022

alan-ghelardi mentioned this pull request Oct 18, 2022

Fix resource deletion and refactor watcher #255

Merged

dibyom mentioned this pull request Oct 18, 2022

TODO - make the location of the keypair configurable. #256

Closed

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2022

tekton-robot merged commit f4189f3 into tektoncd:main Oct 18, 2022

AndrienkoAleksandr mentioned this pull request Dec 14, 2022

Make sa token and certificates path configurable. #283

Merged

khrm reviewed Feb 27, 2023

View reviewed changes

tekton-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Feb 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve coverage of E2E tests #253

Improve coverage of E2E tests #253

alan-ghelardi commented Oct 13, 2022 •

edited

Loading

tekton-robot commented Oct 13, 2022

vdemeester commented Oct 13, 2022

adambkaplan left a comment

adambkaplan Oct 13, 2022

adambkaplan Oct 13, 2022

adambkaplan Oct 13, 2022

adambkaplan Oct 13, 2022

adambkaplan Oct 13, 2022

alan-ghelardi commented Oct 13, 2022

alan-ghelardi commented Oct 13, 2022

adambkaplan left a comment

tekton-robot commented Oct 13, 2022

alan-ghelardi commented Oct 17, 2022

dibyom commented Oct 18, 2022

khrm left a comment

		-keyout "/tmp/tekton-results-key.pem" \
		-out "/tmp/tekton-results-cert.pem" \

		allNamespacesReadAccess = "/tmp/tekton-results/tokens/all-namespaces-read-access"
		singleNamespaceReadAccess = "/tmp/tekton-results/tokens/single-namespace-read-access"

Improve coverage of E2E tests #253

Improve coverage of E2E tests #253

Conversation

alan-ghelardi commented Oct 13, 2022 • edited Loading

What

Additional information

tekton-robot commented Oct 13, 2022

vdemeester commented Oct 13, 2022

adambkaplan left a comment

Choose a reason for hiding this comment

adambkaplan Oct 13, 2022

Choose a reason for hiding this comment

adambkaplan Oct 13, 2022

Choose a reason for hiding this comment

adambkaplan Oct 13, 2022

Choose a reason for hiding this comment

adambkaplan Oct 13, 2022

Choose a reason for hiding this comment

adambkaplan Oct 13, 2022

Choose a reason for hiding this comment

alan-ghelardi commented Oct 13, 2022

alan-ghelardi commented Oct 13, 2022

adambkaplan left a comment

Choose a reason for hiding this comment

tekton-robot commented Oct 13, 2022

alan-ghelardi commented Oct 17, 2022

dibyom commented Oct 18, 2022

khrm left a comment

Choose a reason for hiding this comment

alan-ghelardi commented Oct 13, 2022 •

edited

Loading