test: add unit tests for `pkg/resolution/resource` #6433

l-qing · 2023-03-26T17:53:17Z

fix #6429

I found two minor bugs while writing unit tests 😁：

ownerRefsAreEqual checking owner reference equality to always return false.
- Currently, the owner is originally empty.
- So, there is no impact at the moment.
GenerateDeterministicName not consider params when generating unique names.
- Currently, including the taskrun name in the base name can to some extent avoid conflicts.
- So, there is no impact at the moment.

These two bugs currently do not have any substantial impact.

Changes

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

NONE

tekton-robot · 2023-03-26T17:53:27Z

Hi @l-qing. 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.

l-qing · 2023-03-26T18:00:02Z

/kind bug
/kind flake

tekton-robot · 2023-03-26T18:00:16Z

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

File	Old Coverage	New Coverage
pkg/resolution/resource/crd_resource.go	Do not exist	92.9%
pkg/resolution/resource/name.go	Do not exist	76.2%
pkg/resolution/resource/request.go	Do not exist	100.0%
test/loadfile.go	Do not exist	88.9%

l-qing · 2023-03-26T18:01:35Z

/assign lbernick

tekton-robot · 2023-03-26T18:14:34Z

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

File	Old Coverage	New Coverage
pkg/resolution/resource/crd_resource.go	Do not exist	92.9%
pkg/resolution/resource/name.go	Do not exist	76.2%
pkg/resolution/resource/request.go	Do not exist	100.0%
test/loadfile.go	Do not exist	100.0%

l-qing · 2023-03-26T18:13:37Z

pkg/resolution/common/interface.go

+// ResolverName is the type used for a resolver's name and is mostly
+// used to ensure the function signatures that accept it are clear on the
+// purpose for the given string.
+type ResolverName string


move from:

pipeline/pkg/resolution/resource/resource.go

Lines 26 to 63 in 40763d8

// ResolverName is the type used for a resolver's name and is mostly

// used to ensure the function signatures that accept it are clear on the

// purpose for the given string.

type ResolverName string

// Requester is the interface implemented by a type that knows how to

// submit requests for remote resources.

type Requester interface {

// Submit accepts the name of a resolver to submit a request to

// along with the request itself.

Submit(context.Context, ResolverName, Request) (ResolvedResource, error)

}

// Request is implemented by any type that represents a single request

// for a remote resource. Implementing this interface gives the underlying

// type an opportunity to control properties such as whether the name of

// a request has particular properties, whether the request should be made

// to a specific namespace, and precisely which parameters should be included.

type Request interface {

Name() string

Namespace() string

Params() []pipelinev1beta1.Param

}

// OwnedRequest is implemented by any type implementing Request that also needs

// to express a Kubernetes OwnerRef relationship as part of the request being

// made.

type OwnedRequest interface {

OwnerRef() metav1.OwnerReference

}

// ResolvedResource is implemented by any type that offers a read-only

// view of the data and metadata of a resolved remote resource.

type ResolvedResource interface {

Data() ([]byte, error)

Annotations() map[string]string

Source() *pipelinev1beta1.ConfigSource

}

avoid cycle import. 😁

The test package references these interfaces.

pipeline/test/resolution.go

Line 12 in 40763d8

resolution "github.com/tektoncd/pipeline/pkg/resolution/resource"

Although moving the interface to write unit tests is not reasonable, since the methods in the test package are referenced in multiple places, I can only choose the way to make the minimum changes.

And I also need to use existing methods in the test package, I don't want to duplicate them here.

pkg/resolution/resource/crd_resource.go

pkg/resolution/resource/name.go

test/loadfile.go

test/resolution.go

l-qing · 2023-03-26T18:23:23Z

/remove-kind bug

lbernick

Thanks so much @l-qing! I love that you were able to find two bugs by adding these tests-- that's great!

pkg/resolution/resource/crd_resource_test.go

pkg/resolution/resource/name.go

pkg/resolution/resource/crd_resource_test.go

pkg/resolution/resource/testdata/submit.resolution_request.created.yaml

test/loadfile.go

tekton-robot · 2023-03-27T14:10:57Z

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

File	Old Coverage	New Coverage
pkg/resolution/resource/crd_resource.go	Do not exist	92.9%
pkg/resolution/resource/name.go	Do not exist	76.2%
pkg/resolution/resource/request.go	Do not exist	100.0%
test/loadfile.go	Do not exist	100.0%

tekton-robot · 2023-03-27T15:28:42Z

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

File	Old Coverage	New Coverage
pkg/resolution/resource/crd_resource.go	Do not exist	97.6%
pkg/resolution/resource/name.go	Do not exist	76.2%
pkg/resolution/resource/request.go	Do not exist	100.0%
test/loadfile.go	Do not exist	100.0%

l-qing · 2023-03-27T15:30:55Z

Hi, @lbernick.
I've adjusted the code based on your comments. Could you please take some time to review it again when you're available? Thanks!

l-qing · 2023-03-27T15:39:54Z

/cc @imjasonh @wlynch

tekton-robot · 2023-04-11T23:49: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
pkg/resolution/resource/crd_resource.go	Do not exist	83.3%
pkg/resolution/resource/name.go	Do not exist	76.2%
pkg/resolution/resource/request.go	Do not exist	100.0%

tekton-robot · 2023-04-11T23:52:01Z

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

File	Old Coverage	New Coverage
pkg/resolution/resource/crd_resource.go	Do not exist	83.3%
pkg/resolution/resource/name.go	Do not exist	76.2%
pkg/resolution/resource/request.go	Do not exist	100.0%

l-qing · 2023-04-12T01:14:39Z

/retest

QuanZhang-William · 2023-04-12T17:51:34Z

/assign

tekton-robot · 2023-04-13T07:16:44Z

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
pkg/resolution/resource/crd_resource.go	Do not exist	83.3%
pkg/resolution/resource/name.go	Do not exist	76.2%
pkg/resolution/resource/request.go	Do not exist	100.0%

tekton-robot · 2023-04-13T07:21:39Z

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

File	Old Coverage	New Coverage
pkg/resolution/resource/crd_resource.go	Do not exist	83.3%
pkg/resolution/resource/name.go	Do not exist	76.2%
pkg/resolution/resource/request.go	Do not exist	100.0%

tekton-robot · 2023-04-15T02:49:02Z

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
pkg/resolution/resource/crd_resource.go	Do not exist	83.3%
pkg/resolution/resource/name.go	Do not exist	76.2%
pkg/resolution/resource/request.go	Do not exist	100.0%

tekton-robot · 2023-04-15T02:51:10Z

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

File	Old Coverage	New Coverage
pkg/resolution/resource/crd_resource.go	Do not exist	83.3%
pkg/resolution/resource/name.go	Do not exist	76.2%
pkg/resolution/resource/request.go	Do not exist	100.0%

tekton-robot · 2023-04-17T13:25: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
pkg/resolution/resource/crd_resource.go	Do not exist	83.3%
pkg/resolution/resource/name.go	Do not exist	76.2%
pkg/resolution/resource/request.go	Do not exist	100.0%

tekton-robot · 2023-04-17T13:25:31Z

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

File	Old Coverage	New Coverage
pkg/resolution/resource/crd_resource.go	Do not exist	83.3%
pkg/resolution/resource/name.go	Do not exist	76.2%
pkg/resolution/resource/request.go	Do not exist	100.0%

tekton-robot · 2023-04-18T14:29:50Z

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
pkg/resolution/resource/crd_resource.go	Do not exist	83.3%
pkg/resolution/resource/name.go	Do not exist	76.2%
pkg/resolution/resource/request.go	Do not exist	100.0%

tekton-robot · 2023-04-18T14:32:05Z

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

File	Old Coverage	New Coverage
pkg/resolution/resource/crd_resource.go	Do not exist	83.3%
pkg/resolution/resource/name.go	Do not exist	76.2%
pkg/resolution/resource/request.go	Do not exist	100.0%

tekton-robot · 2023-04-20T05:46:29Z

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
pkg/resolution/resource/crd_resource.go	Do not exist	83.3%
pkg/resolution/resource/name.go	Do not exist	76.2%
pkg/resolution/resource/request.go	Do not exist	100.0%

tekton-robot · 2023-04-20T05:49:44Z

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

File	Old Coverage	New Coverage
pkg/resolution/resource/crd_resource.go	Do not exist	83.3%
pkg/resolution/resource/name.go	Do not exist	76.2%
pkg/resolution/resource/request.go	Do not exist	100.0%

fix tektoncd#6429

tekton-robot · 2023-04-21T09:43:04Z

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
pkg/resolution/resource/crd_resource.go	Do not exist	83.3%
pkg/resolution/resource/name.go	Do not exist	76.2%
pkg/resolution/resource/request.go	Do not exist	100.0%

tekton-robot · 2023-04-21T09:44:18Z

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

File	Old Coverage	New Coverage
pkg/resolution/resource/crd_resource.go	Do not exist	83.3%
pkg/resolution/resource/name.go	Do not exist	76.2%
pkg/resolution/resource/request.go	Do not exist	100.0%

l-qing · 2023-04-21T10:16:12Z

/retest

lbernick · 2023-04-21T13:29:26Z

/lgtm
I don't think there's much benefit to having you continue to rebase; just going to go ahead and merge this

l-qing · 2023-04-21T13:44:20Z

/lgtm I don't think there's much benefit to having you continue to rebase; just going to go ahead and merge this

Thanks!

tekton-robot added the release-note-none Denotes a PR that doesnt merit a release note. label Mar 26, 2023

tekton-robot requested review from imjasonh and wlynch March 26, 2023 17:53

tekton-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 26, 2023

tekton-robot added kind/bug Categorizes issue or PR as related to a bug. kind/flake Categorizes issue or PR as related to a flakey test labels Mar 26, 2023

tekton-robot assigned lbernick Mar 26, 2023

l-qing force-pushed the test/resolution-resource branch from 73d3809 to 277d879 Compare March 26, 2023 18:07

tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 26, 2023

l-qing commented Mar 26, 2023

View reviewed changes

tekton-robot removed the kind/bug Categorizes issue or PR as related to a bug. label Mar 26, 2023

l-qing force-pushed the test/resolution-resource branch from 277d879 to e84e844 Compare March 27, 2023 14:04

lbernick reviewed Mar 27, 2023

View reviewed changes

l-qing force-pushed the test/resolution-resource branch from e84e844 to 0647456 Compare March 27, 2023 15:21

l-qing requested review from lbernick and removed request for imjasonh and wlynch March 27, 2023 15:29

tekton-robot requested a review from imjasonh March 27, 2023 15:39

tekton-robot assigned QuanZhang-William Apr 12, 2023

l-qing force-pushed the test/resolution-resource branch from b68da6b to 6db5768 Compare April 13, 2023 07:11

l-qing force-pushed the test/resolution-resource branch from 6db5768 to d8a29c4 Compare April 15, 2023 02:43

l-qing force-pushed the test/resolution-resource branch from d8a29c4 to 4366720 Compare April 17, 2023 13:19

l-qing force-pushed the test/resolution-resource branch from 4366720 to 439aaff Compare April 18, 2023 14:17

QuanZhang-William removed their assignment Apr 18, 2023

l-qing force-pushed the test/resolution-resource branch from 439aaff to b2311be Compare April 20, 2023 05:41

test: add unit tests for pkg/resolution/resource

656c279

fix tektoncd#6429

l-qing force-pushed the test/resolution-resource branch from b2311be to 656c279 Compare April 21, 2023 09:36

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 21, 2023

tekton-robot merged commit a7424c4 into tektoncd:main Apr 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add unit tests for `pkg/resolution/resource` #6433

test: add unit tests for `pkg/resolution/resource` #6433

l-qing commented Mar 26, 2023 •

edited

Loading

tekton-robot commented Mar 26, 2023

l-qing commented Mar 26, 2023

tekton-robot commented Mar 26, 2023

l-qing commented Mar 26, 2023

tekton-robot commented Mar 26, 2023

l-qing Mar 26, 2023

l-qing Mar 27, 2023

l-qing commented Mar 26, 2023

lbernick left a comment

tekton-robot commented Mar 27, 2023

tekton-robot commented Mar 27, 2023

l-qing commented Mar 27, 2023

l-qing commented Mar 27, 2023

tekton-robot commented Apr 11, 2023

tekton-robot commented Apr 11, 2023

l-qing commented Apr 12, 2023

QuanZhang-William commented Apr 12, 2023

tekton-robot commented Apr 13, 2023

tekton-robot commented Apr 13, 2023

tekton-robot commented Apr 15, 2023

tekton-robot commented Apr 15, 2023

tekton-robot commented Apr 17, 2023

tekton-robot commented Apr 17, 2023

tekton-robot commented Apr 18, 2023

tekton-robot commented Apr 18, 2023

tekton-robot commented Apr 20, 2023

tekton-robot commented Apr 20, 2023

tekton-robot commented Apr 21, 2023

tekton-robot commented Apr 21, 2023

l-qing commented Apr 21, 2023

lbernick commented Apr 21, 2023

l-qing commented Apr 21, 2023

	// ResolverName is the type used for a resolver's name and is mostly
	// used to ensure the function signatures that accept it are clear on the
	// purpose for the given string.
	type ResolverName string

	// Requester is the interface implemented by a type that knows how to
	// submit requests for remote resources.
	type Requester interface {
	// Submit accepts the name of a resolver to submit a request to
	// along with the request itself.
	Submit(context.Context, ResolverName, Request) (ResolvedResource, error)
	}

	// Request is implemented by any type that represents a single request
	// for a remote resource. Implementing this interface gives the underlying
	// type an opportunity to control properties such as whether the name of
	// a request has particular properties, whether the request should be made
	// to a specific namespace, and precisely which parameters should be included.
	type Request interface {
	Name() string
	Namespace() string
	Params() []pipelinev1beta1.Param
	}

	// OwnedRequest is implemented by any type implementing Request that also needs
	// to express a Kubernetes OwnerRef relationship as part of the request being
	// made.
	type OwnedRequest interface {
	OwnerRef() metav1.OwnerReference
	}

	// ResolvedResource is implemented by any type that offers a read-only
	// view of the data and metadata of a resolved remote resource.
	type ResolvedResource interface {
	Data() ([]byte, error)
	Annotations() map[string]string
	Source() *pipelinev1beta1.ConfigSource
	}

test: add unit tests for pkg/resolution/resource #6433

test: add unit tests for pkg/resolution/resource #6433

Conversation

l-qing commented Mar 26, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Mar 26, 2023

l-qing commented Mar 26, 2023

tekton-robot commented Mar 26, 2023

l-qing commented Mar 26, 2023

tekton-robot commented Mar 26, 2023

l-qing Mar 26, 2023

Choose a reason for hiding this comment

l-qing Mar 27, 2023

Choose a reason for hiding this comment

l-qing commented Mar 26, 2023

lbernick left a comment

Choose a reason for hiding this comment

tekton-robot commented Mar 27, 2023

tekton-robot commented Mar 27, 2023

l-qing commented Mar 27, 2023

l-qing commented Mar 27, 2023

tekton-robot commented Apr 11, 2023

tekton-robot commented Apr 11, 2023

l-qing commented Apr 12, 2023

QuanZhang-William commented Apr 12, 2023

tekton-robot commented Apr 13, 2023

tekton-robot commented Apr 13, 2023

tekton-robot commented Apr 15, 2023

tekton-robot commented Apr 15, 2023

tekton-robot commented Apr 17, 2023

tekton-robot commented Apr 17, 2023

tekton-robot commented Apr 18, 2023

tekton-robot commented Apr 18, 2023

tekton-robot commented Apr 20, 2023

tekton-robot commented Apr 20, 2023

tekton-robot commented Apr 21, 2023

tekton-robot commented Apr 21, 2023

l-qing commented Apr 21, 2023

lbernick commented Apr 21, 2023

l-qing commented Apr 21, 2023

test: add unit tests for `pkg/resolution/resource` #6433

test: add unit tests for `pkg/resolution/resource` #6433

l-qing commented Mar 26, 2023 •

edited

Loading