Add unit tests for bundle resolver #5704

chuangw6 · 2022-11-01T13:49:14Z

Changes

/kind misc

Prior, remote bundle solver didn't have unit test for the resolve method.

In this commit, we added the unit test by setup a local test registry.
Note: Some commonly used testing helpers are extracted to
pkg/resolution/resolver/framework/testing/fakecontroller.go file.

Signed-off-by: Chuang Wang chuangw@google.com

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs included if any changes are user facing
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Add unit test for bundle resolver

tekton-robot · 2022-11-01T13:55:23Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/resolution/resolver/bundle/bundle.go	0.0%	70.6%	70.6
pkg/resolution/resolver/bundle/resolver.go	50.0%	96.2%	46.2

abayer · 2022-11-01T14:15:14Z

/approve

tekton-robot · 2022-11-01T14:33:59Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/resolution/resolver/bundle/bundle.go	0.0%	70.6%	70.6
pkg/resolution/resolver/bundle/resolver.go	50.0%	96.2%	46.2

chuangw6 · 2022-11-01T14:37:47Z

/retest

tekton-robot · 2022-11-01T15:12:12Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/resolution/resolver/bundle/bundle.go	0.0%	72.1%	72.1
pkg/resolution/resolver/bundle/resolver.go	50.0%	96.2%	46.2

pkg/resolution/resolver/bundle/resolver.go

pkg/resolution/resolver/bundle/resolver_test.go

tekton-robot · 2022-11-01T15:50:46Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/resolution/resolver/bundle/bundle.go	0.0%	72.1%	72.1
pkg/resolution/resolver/bundle/resolver.go	50.0%	95.8%	45.8

tekton-robot · 2022-11-01T18:22:17Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/resolution/resolver/bundle/bundle.go	0.0%	72.1%	72.1
pkg/resolution/resolver/bundle/resolver.go	50.0%	95.8%	45.8

Yongxuanzhang

Thanks! This lgtm. Feel free to resolve my comments

Yongxuanzhang · 2022-11-01T21:42:17Z

pkg/resolution/resolver/framework/testing/fakecontroller.go

@@ -169,3 +171,26 @@ func ensureConfigurationConfigMapsExist(d *test.Data) {
 		})
 	}
 }
+
+// CreateResolutionRequestStatusWithData returns a ResolutionRequestStatus with the resolved content.
+func CreateResolutionRequestStatusWithData(content []byte) *v1beta1.ResolutionRequestStatus {


I'm not sure if having those files in the fakecontroller.go file is good or not. 😂 Or they could be in a new separate file? Those functions seem not controller related...?

SGTM! Created a new file named resolutionrequest.go, and they are now living in this new file :D PTAL.

I hope I could have the lgtm permission 😂

tekton-robot · 2022-11-01T21:52:41Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/resolution/resolver/bundle/bundle.go	0.0%	72.1%	72.1
pkg/resolution/resolver/bundle/resolver.go	50.0%	95.8%	45.8

tekton-robot · 2022-11-04T18:55:39Z

@chuangw6: GitHub didn't allow me to assign the following users: wl.

Note that only tektoncd members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @wl

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.

chuangw6 · 2022-11-04T18:56:12Z

/assign @wlynch

wlynch · 2022-11-01T14:46:28Z

pkg/resolution/resolver/bundle/resolver_test.go

+				t.Fatalf("could not push image: %#v", err)
+			}
+			// set bundle param to be the image ref
+			tc.args.bundle = ref


It doesn't look like this is ever set in args. Let's just set it here. (same with service account - it doesn't look like it's ever used).

It doesn't look like this is ever set in args. Let's just set it here.

Sounds good. I pre-populated the oci registry with images, and the ref is used for the args.bundle in the test table. PTAL.

same with service account - it doesn't look like it's ever used

Yes, service account is not used in the test, but the param validation expects this to be set somewhere either from param or configmap, otherwise the param validation will fail. That's why we added a placeholder service account field in the configmap. PTAL and let me know wdyt. Thanks!

wlynch · 2022-11-01T14:47:25Z

pkg/resolution/resolver/bundle/resolver_test.go

+
+			ctx, _ := ttesting.SetupFakeContext(t)
+
+			resolver := &Resolver{


Doesn't look like this is ever modified. Hoist out of the loop?

Done. Thanks!

wlynch · 2022-11-04T20:45:36Z

pkg/resolution/resolver/bundle/resolver_test.go

+			expectedErrMessage: fmt.Sprintf("contained more than the maximum %d allow objects", MaximumBundleObjects),
+		}, {
+			name:   "single-task-no-version",
+			objs:   []runtime.Object{&pipelinev1beta1.Task{TypeMeta: metav1.TypeMeta{Kind: "task"}, ObjectMeta: metav1.ObjectMeta{Name: "foo"}}},


Similar to the repo tests - you could probably simplify this by just prepopulating a single OCI test registry with a few different kinds of bundles and just fetch the different values.

By decoupling it from the CRDs this also lets you setup some other interesting tests like what if you try to fetch a valid OCI artifact that isn't a Tekton bundle? 🤔

Similar to the repo tests - you could probably simplify this by just prepopulating a single OCI test registry with a few different kinds of bundles and just fetch the different values.

Good idea. Done. PTAL. Thanks

By decoupling it from the CRDs this also lets you setup some other interesting tests like what if you try to fetch a valid OCI artifact that isn't a Tekton bundle? 🤔

This is interesting. Just play around a bit. It seems like the implementation code will checkImageCompliance after retrieving the image by only checking required annotations and the number of layers. Therefore,

1). if we want to create a valid but non-tekton oci artifact without those annotations, the existing tests i.e. single-task-no-version basically cover that.
2). if we want to create a valid but non-tekton oci artifact with those required annotations, the resolution will succeed.

sounds a bit odd b/c a valid oci artifact that isn't a tekton bundle normally shouldn't have those required annotations in its layers. Please let me know what you think.

wlynch · 2022-11-04T20:47:19Z

pkg/resolution/resolver/framework/testing/resolutionrequest.go

+ limitations under the License.
+*/
+
+package testing


Make this an internal package.

Done. Put it under pkg/resolution/resolver/internal. Thank you for the suggestion!!

wlynch · 2022-11-04T20:52:12Z

pkg/resolution/resolver/bundle/resolver_test.go

+				expectedStatus = tc.expectedStatus.DeepCopy()
+				if tc.expectedErrMessage == "" {
+					reqParams := make(map[string]string)
+					for _, p := range request.Spec.Params {


Same as the repo tests, it looks like we're feeding in request data into the expected response. Can we simplify the comparisons here so it's easier to see what's being compared and what fields we're expecting to be returned for each request?

Good point. Done. Please take a look.

One possible direction to make it even more clear to see what we expect is to explicitly declare what is expected annotation and source other than the data in the ResolutionRequestStatus in the test table instead of inside the for loop. But I guess the downside is that the test table will be too verbose. Not sure if we want to go down that path. If so, happy to do it in another pr (b/c this might need to change the util helper CreateResolutionRequestStatusWithData's interface, and therefore change tests in gitresolver).

tekton-robot · 2022-11-08T18:42:35Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/resolution/resolver/bundle/bundle.go	0.0%	72.1%	72.1
pkg/resolution/resolver/bundle/params.go	78.1%	90.6%	12.5
pkg/resolution/resolver/bundle/resolver.go	50.0%	95.8%	45.8

tekton-robot · 2022-11-08T19:56:08Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/resolution/resolver/bundle/bundle.go	0.0%	72.1%	72.1
pkg/resolution/resolver/bundle/params.go	78.1%	90.6%	12.5
pkg/resolution/resolver/bundle/resolver.go	50.0%	95.8%	45.8

tekton-robot · 2022-11-08T20:02:59Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/resolution/resolver/bundle/bundle.go	0.0%	72.1%	72.1
pkg/resolution/resolver/bundle/params.go	78.1%	90.6%	12.5
pkg/resolution/resolver/bundle/resolver.go	50.0%	95.8%	45.8

Prior, remote bundle solver didn't have unit test for the resolve method. In this commit, we added the unit test by setup a local test registry. Note: Some commonly used testing helpers are extracted to pkg/resolution/resolver/framework/testing/fakecontroller.go file. Signed-off-by: Chuang Wang <chuangw@google.com>

tekton-robot · 2022-11-08T20:40:54Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/resolution/resolver/bundle/bundle.go	0.0%	72.1%	72.1
pkg/resolution/resolver/bundle/params.go	78.1%	90.6%	12.5
pkg/resolution/resolver/bundle/resolver.go	50.0%	95.8%	45.8

wlynch

💯 Looks great!

/lgtm

tekton-robot · 2022-11-09T19:03:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abayer, vdemeester, wlynch, Yongxuanzhang

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~~ [abayer,vdemeester]

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

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/misc Categorizes issue or PR as a miscellaneuous one. labels Nov 1, 2022

tekton-robot requested review from jerop and pritidesai November 1, 2022 13:49

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 1, 2022

chuangw6 mentioned this pull request Nov 1, 2022

Set ConfigSource in bundleresolver #5684

Merged

7 tasks

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

chuangw6 force-pushed the unit-test-bundle-resolver branch from d11e604 to d2f35b9 Compare November 1, 2022 14:27

chuangw6 force-pushed the unit-test-bundle-resolver branch from d2f35b9 to 44ef0b5 Compare November 1, 2022 15:07

Yongxuanzhang reviewed Nov 1, 2022

View reviewed changes

pkg/resolution/resolver/bundle/resolver.go Outdated Show resolved Hide resolved

pkg/resolution/resolver/bundle/resolver_test.go Outdated Show resolved Hide resolved

pkg/resolution/resolver/bundle/resolver_test.go Outdated Show resolved Hide resolved

chuangw6 force-pushed the unit-test-bundle-resolver branch from 44ef0b5 to 1d27883 Compare November 1, 2022 15:44

chuangw6 force-pushed the unit-test-bundle-resolver branch from 1d27883 to fd836e8 Compare November 1, 2022 18:13

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 1, 2022

chuangw6 force-pushed the unit-test-bundle-resolver branch from fd836e8 to 0aca5ff Compare November 1, 2022 18:16

Yongxuanzhang approved these changes Nov 1, 2022

View reviewed changes

chuangw6 force-pushed the unit-test-bundle-resolver branch from 0aca5ff to 1c79659 Compare November 1, 2022 21:47

chuangw6 requested review from Yongxuanzhang and removed request for pritidesai and jerop November 1, 2022 21:50

vdemeester approved these changes Nov 4, 2022

View reviewed changes

tekton-robot assigned wlynch Nov 4, 2022

wlynch requested changes Nov 7, 2022

View reviewed changes

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2022

chuangw6 force-pushed the unit-test-bundle-resolver branch 2 times, most recently from a261a1a to 02e1ee0 Compare November 8, 2022 18:37

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2022

chuangw6 force-pushed the unit-test-bundle-resolver branch from 02e1ee0 to 9afacdd Compare November 8, 2022 19:51

chuangw6 force-pushed the unit-test-bundle-resolver branch from 9afacdd to 2052a03 Compare November 8, 2022 19:57

chuangw6 force-pushed the unit-test-bundle-resolver branch from 2052a03 to 695b407 Compare November 8, 2022 20:34

chuangw6 force-pushed the unit-test-bundle-resolver branch from 695b407 to dad54cd Compare November 8, 2022 20:36

chuangw6 requested review from wlynch and removed request for Yongxuanzhang and wlynch November 8, 2022 20:44

wlynch approved these changes Nov 9, 2022

View reviewed changes

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

tekton-robot merged commit 57226ac into tektoncd:main Nov 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit tests for bundle resolver #5704

Add unit tests for bundle resolver #5704

chuangw6 commented Nov 1, 2022 •

edited

Loading

tekton-robot commented Nov 1, 2022

abayer commented Nov 1, 2022

tekton-robot commented Nov 1, 2022

chuangw6 commented Nov 1, 2022

tekton-robot commented Nov 1, 2022

tekton-robot commented Nov 1, 2022

tekton-robot commented Nov 1, 2022

Yongxuanzhang left a comment

Yongxuanzhang Nov 1, 2022

chuangw6 Nov 1, 2022

Yongxuanzhang Nov 1, 2022

Yongxuanzhang Nov 1, 2022

tekton-robot commented Nov 1, 2022

tekton-robot commented Nov 4, 2022

chuangw6 commented Nov 4, 2022

wlynch Nov 1, 2022

chuangw6 Nov 8, 2022

wlynch Nov 1, 2022

chuangw6 Nov 8, 2022

wlynch Nov 4, 2022

chuangw6 Nov 8, 2022

wlynch Nov 4, 2022

chuangw6 Nov 8, 2022

wlynch Nov 4, 2022

chuangw6 Nov 8, 2022 •

edited

Loading

tekton-robot commented Nov 8, 2022

tekton-robot commented Nov 8, 2022

tekton-robot commented Nov 8, 2022

tekton-robot commented Nov 8, 2022

wlynch left a comment

tekton-robot commented Nov 9, 2022


		ctx, _ := ttesting.SetupFakeContext(t)

		resolver := &Resolver{

Add unit tests for bundle resolver #5704

Add unit tests for bundle resolver #5704

Conversation

chuangw6 commented Nov 1, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Nov 1, 2022

abayer commented Nov 1, 2022

tekton-robot commented Nov 1, 2022

chuangw6 commented Nov 1, 2022

tekton-robot commented Nov 1, 2022

tekton-robot commented Nov 1, 2022

tekton-robot commented Nov 1, 2022

Yongxuanzhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Nov 1, 2022

tekton-robot commented Nov 4, 2022

chuangw6 commented Nov 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuangw6 Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

tekton-robot commented Nov 8, 2022

tekton-robot commented Nov 8, 2022

tekton-robot commented Nov 8, 2022

tekton-robot commented Nov 8, 2022

wlynch left a comment

Choose a reason for hiding this comment

tekton-robot commented Nov 9, 2022

chuangw6 commented Nov 1, 2022 •

edited

Loading

chuangw6 Nov 8, 2022 •

edited

Loading