Add core Reconciler testing and fake resolver framework functions #51

abayer · 2022-05-18T20:56:54Z

Add core Reconciler testing and fake resolver framework functions

Fixes #39

Tests are added for pkg/resolver/framework/reconciler.go and pkg/reconciler/resolutionrequest/resolution.request.go, as well as the new framework for testing Resolvers in pkg/resolver/framework/testing, with tests using that in gitresolver/pkg/git/resolver_test.go and docs/resolver-template/cmd/demoresolver/main_test.go.

I didn't add Resolver tests for the bundle resolver, because the tooling to actually do so with fake bundles etc already exists in Pipeline, and it's not worth copy-pasting that tooling over here when we're gonna move over to Pipeline soon enough anyway!

In addition, e2e tests are reorganized a bit, and I added some other tooling derived from what we've got in Pipeline.

abayer · 2022-05-18T20:59:28Z

pkg/resolver/framework/fakecontroller.go

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


I had to put the fake controller/resolver stuff in this package in order to use them in reconciler_test.go thanks to the eternal fun of circular dependencies, but the fake controller in particular is intended to be used by real Resolver implementations, and the fake resolver will (eventually!) end up being used in Pipeline's unit tests once we merge Resolution into Pipeline.

Also, @chitrangpatel, I think this and fakeresolver.go may be helpful in writing tests for pkg/resource/crd_resource.go.

abayer · 2022-05-18T21:00:26Z

test/diff/print.go

+// in a consistent format for reuse across all of our tests. This
+// func assumes that the order of arguments passed to cmp.Diff was
+// (want, got) or, in other words, the expectedResult then the actualResult.
+func PrintWantGot(diff string) string {


This and test/names/generate.go are copy-pasted directly from Pipeline, and will go away once we've folded Resolution into Pipeline.

abayer · 2022-05-18T21:01:01Z

test/controller.go

+// SeedTestData returns Clients and Informers populated with the
+// given Data.
+// nolint: revive
+func SeedTestData(t *testing.T, ctx context.Context, d Data) (Clients, Informers) {


This is basically a stripped down copy of Pipeline's test/controller.go, and will go away once we combine the repos.

abayer · 2022-05-18T21:02:06Z

Makefile

@@ -0,0 +1,200 @@
+MODULE   = $(shell env GO111MODULE=on $(GO) list -m)


What can I say, I like consistent Makefiles across projects I'm working on. =) This, like a number of other things I've called out, will go away once we merge Resolution into Pipeline.

abayer · 2022-05-18T21:05:27Z

cc @chitrangpatel, @dibyom, @vdemeester, @lbernick

I've got more I'm going to add to this - an "acceptance test" for Resolver implementations to make sure they correctly satisfy the Resolver interface and refining what's in pkg/resolver/framework/reconciler_test.go into a helper function that Resolver implementations can use for their own testing are what comes to mind currently, but this is a start.

Oh, and my apologies for the size of the PR - that's basically all vendor going a bit bananas.

vdemeester

/retest

abayer · 2022-05-19T14:20:54Z

bundleresolver/cmd/bundleresolver/main.go

@@ -31,8 +33,9 @@ import (
 const timeoutDuration = time.Minute

 func main() {
-	sharedmain.Main("controller",
-		framework.NewController(context.Background(), &resolver{}),
+	ctx := filteredinformerfactory.WithSelectors(context.Background(), v1alpha1.ManagedByLabelKey)


I don't know why this didn't barf with Unable to fetch labelkey from context previously, but it started doing that in this PR until I added this and changed the next lines here and in the other Resolver main.gos.

abayer · 2022-05-19T17:59:48Z

pkg/resolver/framework/testing/reconciler_test.go

+	_ "knative.dev/pkg/system/testing" // Setup system.Namespace()
+)
+
+func TestReconcile(t *testing.T) {


Ok, so...this is in pkg/resolver/framework/testing because fakecontroller.go makes the Resolver binaries barf with /ko-app/bundleresolver flag redefined: cluster if fakecontroller.go is in pkg/resolver/framework - some Knative testing magic ends up in the resulting Resolver binary, calling ClientConfig.InitFlags(flag.CommandLine) at init time via, well, magic, resulting in the later call via sharedmain.MainWithContext failing.

So if I wanted to use the fake reconciler framework elsewhere - i.e., in tests for bundleresolver etc - I have to have the fake controller/reconciler tooling exported (so not in _test.go) but not in the same package as stuff the Resolver main.gos import. I could just switch this reconciler_test.go to not use the fake framework but instead its own copy-pasted equivalent, but...yeah. I'm waiting for someone else to come up with ideas. =)

Ok, 6721eae seems to fix it, at least at first glance - moving pkg/clients.go to pkg/clients_test.go got rid of the import chain that led to knative.dev/pkg/test ending up in the Resolver binaries, and I am pretty sure it won't break anything in the integration tests...

Well, it did break something in the integration tests... =)

Ok, I gave up, at least for now, and copy-pasted the testing framework code around. Ah well.

abayer · 2022-05-24T21:51:34Z

/retest

chitrangpatel · 2022-05-25T18:48:35Z

nice to see all the tests pass 😄 . Is this still a WIP @abayer ? I wanted to write some tests for resources but wanted to pull in this work first.

Part of tektoncd#39 Tests are added for `pkg/resolver/framework/reconciler.go` and `pkg/reconciler/resolutionrequest/resolution.request.go`, as well as the new framework for testing `Resolver`s in `pkg/resolver/framework/testing`, with tests using that in `gitresolver/pkg/git/resolver_test.go` and `docs/resolver-template/cmd/demoresolver/main_test.go`. In addition, e2e tests are reorganized a bit, and I added some other tooling derived from what we've got in Pipeline. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

abayer · 2022-05-31T17:05:37Z

Ok, took this out of WIP. @vdemeester, @jerop, @dibyom, @chitrangpatel - I'd really appreciate reviews. =)

chitrangpatel · 2022-05-31T19:15:24Z

/approved

vdemeester · 2022-06-01T09:22:18Z

pkg/names/generate.go

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


Isn't this the same package as tektoncd/pipeline/pkg/names ?
My guess is to duplicate here and just remove once we merge the repository back into pipeline ?

tekton-robot · 2022-06-01T09:24:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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

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

abayer · 2022-06-01T14:39:46Z

@jerop @chitrangpatel - I could use an /lgtm. =)

chitrangpatel · 2022-06-01T14:47:01Z

/lgtm

tekton-robot · 2022-06-01T14:47:09Z

@chitrangpatel: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

chitrangpatel · 2022-06-01T14:50:19Z

@jerop @chitrangpatel - I could use an /lgtm. =)

I'm unable to give it. I get the message: @chitrangpatel: changing LGTM is restricted to collaborators

vdemeester · 2022-06-01T14:54:58Z

@jerop @chitrangpatel - I could use an /lgtm. =)

I'm unable to give it. I get the message: @chitrangpatel: changing LGTM is restricted to collaborators

Hum, something might have failed with the org sync.. Let's me check… In the meantime…
/lgtm

chitrangpatel · 2022-06-01T15:06:18Z

/lgtm

Part of TEP-0110 Update Git Resolver example to use ```revision``` and ```pathInRepo``` fields to reflect the changes in [TEP-0110: Add git revision resolution suppot in Git Resolver](tektoncd/resolution#75) and [Add core Reconciler testing and fake resolver framework functions](tektoncd/resolution#51)

Part of TEP-0115 Update Git Resolver example to use ```revision``` and ```pathInRepo``` fields to reflect the changes in [TEP-0110: Add git revision resolution suppot in Git Resolver](tektoncd/resolution#75) and [Add core Reconciler testing and fake resolver framework functions](tektoncd/resolution#51)

Part of TEP-0115 Update Git Resolver example to use ```revision``` and ```pathInRepo``` fields to reflect the changes in [TEP-0115: Add git revision resolution suppot in Git Resolver](tektoncd/resolution#75) and [Add core Reconciler testing and fake resolver framework functions](tektoncd/resolution#51)

Part of TEP-0115 Update Git Resolver documentation to use "revision" and "pathInRepo" fields to reflect the changes in [TEP-0115: Add git revision resolution support in Git Resolver](tektoncd#75) and [Add core Reconciler testing and fake resolver framework functions](tektoncd#51)

Part of TEP-0115 Update git resolver example to use "revision" and "pathInRepo" fields to reflect the changes in [TEP-0115: Add git revision resolution suppot in Git Resolver](tektoncd/resolution#75) and [Add core Reconciler testing and fake resolver framework functions](tektoncd/resolution#51)

Part of TEP-0115 Update Git Resolver documentation to use "revision" and "pathInRepo" fields to reflect the changes in [TEP-0115: Add git revision resolution support in Git Resolver](#75) and [Add core Reconciler testing and fake resolver framework functions](#51)

Part of TEP-0115 Update git resolver example to use "revision" and "pathInRepo" fields to reflect the changes in [TEP-0115: Add git revision resolution suppot in Git Resolver](tektoncd/resolution#75) and [Add core Reconciler testing and fake resolver framework functions](tektoncd/resolution#51)

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 18, 2022

tekton-robot requested review from dibyom and a user May 18, 2022 20:57

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 18, 2022

abayer commented May 18, 2022

View reviewed changes

vdemeester approved these changes May 19, 2022

View reviewed changes

abayer force-pushed the reconciler-tests branch from bf83c04 to 5f111d0 Compare May 19, 2022 14:19

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

abayer commented May 19, 2022

View reviewed changes

abayer force-pushed the reconciler-tests branch 3 times, most recently from 653919d to 666b136 Compare May 19, 2022 17:49

abayer commented May 19, 2022

View reviewed changes

abayer force-pushed the reconciler-tests branch from cb3e55e to 137e577 Compare May 25, 2022 15:45

abayer force-pushed the reconciler-tests branch from a5a308e to 8723c6f Compare May 31, 2022 17:04

abayer changed the title ~~WIP: Add core Reconciler testing and fake resolver framework functions~~ Add core Reconciler testing and fake resolver framework functions May 31, 2022

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 31, 2022

vdemeester approved these changes Jun 1, 2022

View reviewed changes

tekton-robot assigned chitrangpatel Jun 1, 2022

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

tekton-robot merged commit d93542e into tektoncd:main Jun 1, 2022

abayer mentioned this pull request Jun 1, 2022

Create test helpers for downstream projects to use #10

Closed

QuanZhang-William mentioned this pull request Jul 29, 2022

TEP-0115: Update Git Resolver example to use revision and pathInRepo fields tektoncd/pipeline#5238

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add core Reconciler testing and fake resolver framework functions #51

Add core Reconciler testing and fake resolver framework functions #51

abayer commented May 18, 2022 •

edited

Loading

abayer May 18, 2022

abayer May 18, 2022

abayer May 18, 2022

abayer May 18, 2022

abayer May 18, 2022

abayer commented May 18, 2022 •

edited

Loading

vdemeester left a comment

abayer May 19, 2022

abayer May 19, 2022

abayer May 23, 2022

abayer May 23, 2022

abayer May 23, 2022

abayer commented May 24, 2022

chitrangpatel commented May 25, 2022 •

edited

Loading

abayer commented May 31, 2022

chitrangpatel commented May 31, 2022

vdemeester Jun 1, 2022

abayer Jun 1, 2022

tekton-robot commented Jun 1, 2022

abayer commented Jun 1, 2022

chitrangpatel commented Jun 1, 2022

tekton-robot commented Jun 1, 2022

chitrangpatel commented Jun 1, 2022

vdemeester commented Jun 1, 2022

chitrangpatel commented Jun 1, 2022

		@@ -0,0 +1,200 @@
		MODULE = $(shell env GO111MODULE=on $(GO) list -m)

Add core Reconciler testing and fake resolver framework functions #51

Add core Reconciler testing and fake resolver framework functions #51

Conversation

abayer commented May 18, 2022 • edited Loading

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

abayer commented May 18, 2022 • edited Loading

vdemeester 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

Choose a reason for hiding this comment

abayer commented May 24, 2022

chitrangpatel commented May 25, 2022 • edited Loading

abayer commented May 31, 2022

chitrangpatel commented May 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jun 1, 2022

abayer commented Jun 1, 2022

chitrangpatel commented Jun 1, 2022

tekton-robot commented Jun 1, 2022

chitrangpatel commented Jun 1, 2022

vdemeester commented Jun 1, 2022

chitrangpatel commented Jun 1, 2022

abayer commented May 18, 2022 •

edited

Loading

abayer commented May 18, 2022 •

edited

Loading

chitrangpatel commented May 25, 2022 •

edited

Loading