Refactor sink test #882

dibyom · 2020-12-30T17:59:23Z

Changes

There are two commits in this PR. It's pretty big but a lot of it is deleting duplicated code. The largest change is to take the many TestHandle* tests that all had the same setup steps and combine them into a single table driven test with multiple test cases.

There were a lost of TestHandle* tests with the same copy pasta making
it hard to read/modify e.g. they all copied the same code to create
PipelineResource templates, used the same eventBody etc.

combine most TestHandle into a table driven test
use v1beta1 TaskRuns instead of PipelineResources for all tests
add a func for creating test servers with core/webhook interceptors
refactor test.HMACHeader to take in testing.TB
use switch instead of if/else to reduce verbosity in TestHandle_OverrideAuth
use test.RawExtension helper for creating triggertemplates

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

NONE

tekton-robot · 2020-12-30T18:00:33Z

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

File	Old Coverage	New Coverage	Delta
pkg/interceptors/interceptors.go	90.0%	92.2%	2.2
pkg/sink/sink.go	78.1%	78.5%	0.5
test/controller.go	72.9%	73.1%	0.2
test/resources.go	0.0%	42.9%	42.9
test/signature.go	80.0%	83.3%	3.3

tekton-robot · 2020-12-30T23:02:18Z

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

File	Old Coverage	New Coverage	Delta
pkg/interceptors/interceptors.go	90.0%	92.2%	2.2
pkg/sink/sink.go	78.1%	78.5%	0.5
test/controller.go	72.9%	73.1%	0.2
test/resources.go	0.0%	42.9%	42.9
test/signature.go	80.0%	83.3%	3.3

tekton-robot · 2021-01-06T03:10:29Z

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

File	Old Coverage	New Coverage	Delta
pkg/interceptors/interceptors.go	90.0%	92.2%	2.2
pkg/sink/sink.go	78.1%	78.1%	0.1
test/controller.go	72.9%	73.1%	0.2
test/resources.go	0.0%	42.9%	42.9
test/signature.go	80.0%	83.3%	3.3

tekton-robot · 2021-01-06T16:40:57Z

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

File	Old Coverage	New Coverage	Delta
pkg/interceptors/interceptors.go	90.0%	92.1%	2.1
pkg/sink/sink.go	78.1%	78.4%	0.3
test/controller.go	72.9%	73.1%	0.2
test/resources.go	0.0%	42.9%	42.9
test/signature.go	80.0%	83.3%	3.3

tekton-robot · 2021-01-07T20:30:02Z

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

File	Old Coverage	New Coverage	Delta
test/controller.go	72.9%	73.1%	0.2
test/resources.go	0.0%	42.9%	42.9
test/signature.go	80.0%	83.3%	3.3

bigkevmcd · 2021-01-12T16:47:30Z

/lgtm

test/resources.go

savitaashture

looks good to me 👍

minor question on attribute position change

tekton-robot · 2021-01-12T20:33:41Z

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

File	Old Coverage	New Coverage	Delta
test/controller.go	72.9%	73.1%	0.2
test/resources.go	0.0%	42.9%	42.9
test/signature.go	80.0%	83.3%	3.3

savitaashture

/lgtm

MarcelMue

Just some comments as I try to get into the project more.

pkg/sink/sink_test.go

MarcelMue · 2021-01-13T11:50:15Z

pkg/sink/sink_test.go

+		// want is the resulting TaskRun(s) created by the EventListener
+		want []pipelinev1.TaskRun
+	}{{
+		name: "single trigger embedded within EventListener",


Are there any conventions on test case naming? I generally like this style of naming but have noticed that it is inconsistent throughout the triggers project.

no hard and fast rule. Ideally the name would be both descriptive and short but if not possible, I prefer a more descriptive but longer name over a shorter one (though you are right we haven't been very consistent about this).

MarcelMue · 2021-01-13T11:52:54Z

pkg/sink/sink_test.go

+			// Check right resources were created.
+			got := toTaskRun(t, dynamicClient.Actions())
+			// Sort actions (we do not know what order they executed in)
+			if diff := cmp.Diff(tc.want, got, cmpopts.SortSlices(func(x, y pipelinev1.TaskRun) bool {


Could this be split into two lines? Otherwise the comment is not really true as we sort and compare in a single line. It would make the code more readable / easier to follow IMO.

Updated the comment -- we don't technically sort the taskRuns ourselves, cmp.Diff has an option to sortSlices, we just pass it a function that it can use to compare the TaskRuns when sorting.

MarcelMue · 2021-01-13T11:57:17Z

pkg/sink/sink_test.go

-			"secretKey": []byte("secret"),
-		},
-	}
+// Setup for TestHandleEvent_AuthOverride


Why are these declared in the middle of the file as compared to either at the top or in inside the test func?

We can't put it inside the TestHandleEvent_AuthOverride since these variables are used in the OverrideAuthentication method that we implement for the fakeAuth type below. We could put these at the top of the file too but given that these are only used for the test below, we declare them close to where they are being used.

tekton-robot · 2021-01-13T22:15:55Z

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

File	Old Coverage	New Coverage	Delta
test/controller.go	72.9%	73.1%	0.2
test/resources.go	0.0%	42.9%	42.9
test/signature.go	80.0%	83.3%	3.3

MarcelMue · 2021-01-14T10:26:28Z

/lgtm

tekton-robot · 2021-01-14T10:26:35Z

@MarcelMue: 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.

savitaashture · 2021-01-14T17:10:50Z

/lgtm

khrm

/lgtm

khrm · 2021-01-15T12:53:06Z

/approve

tekton-robot · 2021-01-15T12:53:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: khrm

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

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

tekton-robot · 2021-01-15T16:40:56Z

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

File	Old Coverage	New Coverage	Delta
test/controller.go	72.9%	73.1%	0.2
test/resources.go	0.0%	42.9%	42.9
test/signature.go	80.0%	83.3%	3.3

There were a lost of TestHandle* tests with the same copy pasta making it hard to read/modify e.g. they all copied the same code to create PipelineResource templates, used the same eventBody etc. 1. combine most TestHandle into a table driven test 2. use v1beta1 TaskRuns instead of PipelineResources for all tests 3. add a func for creating test servers with core/webhook interceptors 4. refactor test.HMACHeader to take in testing.TB 5. use switch instead of if/else to reduce verbosity in TestHandle_OverrideAuth 6. use test.RawExtension helper for creating triggertemplates Signed-off-by: Dibyo Mukherjee <[email protected]>

tekton-robot · 2021-01-15T18:50:27Z

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

File	Old Coverage	New Coverage	Delta
test/controller.go	72.9%	73.1%	0.2
test/resources.go	0.0%	42.9%	42.9
test/signature.go	80.0%	83.3%	3.3

MarcelMue · 2021-01-15T19:04:02Z

/lgtm

tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Dec 30, 2020

tekton-robot requested review from ncskier and savitaashture December 30, 2020 17:59

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 30, 2020

dibyom force-pushed the refactor_sink_test branch from 58580a3 to 639ef65 Compare December 30, 2020 23:00

dibyom force-pushed the refactor_sink_test branch from 639ef65 to a589532 Compare January 6, 2021 03:09

dibyom force-pushed the refactor_sink_test branch from a589532 to bdd91ea Compare January 6, 2021 16:39

dibyom force-pushed the refactor_sink_test branch from bdd91ea to 43be775 Compare January 7, 2021 20:28

dibyom marked this pull request as ready for review January 7, 2021 20:29

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 Jan 7, 2021

dibyom assigned khrm and savitaashture Jan 7, 2021

dibyom added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Jan 7, 2021

tekton-robot assigned bigkevmcd Jan 12, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2021

savitaashture reviewed Jan 12, 2021

View reviewed changes

test/resources.go Outdated Show resolved Hide resolved

savitaashture reviewed Jan 12, 2021

View reviewed changes

dibyom force-pushed the refactor_sink_test branch from 43be775 to 3c07486 Compare January 12, 2021 20:32

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2021

savitaashture reviewed Jan 13, 2021

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2021

MarcelMue reviewed Jan 13, 2021

View reviewed changes

dibyom force-pushed the refactor_sink_test branch from 3c07486 to 9f8452a Compare January 13, 2021 22:14

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 14, 2021

khrm reviewed Jan 15, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 15, 2021

dibyom force-pushed the refactor_sink_test branch from 9f8452a to ac2543f Compare January 15, 2021 16:39

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2021

dibyom force-pushed the refactor_sink_test branch from ac2543f to 02cfee5 Compare January 15, 2021 18:49

tekton-robot assigned MarcelMue Jan 15, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2021

tekton-robot merged commit ee784d4 into tektoncd:master Jan 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor sink test #882

Refactor sink test #882

dibyom commented Dec 30, 2020 •

edited

Loading

tekton-robot commented Dec 30, 2020

tekton-robot commented Dec 30, 2020

tekton-robot commented Jan 6, 2021

tekton-robot commented Jan 6, 2021

tekton-robot commented Jan 7, 2021

bigkevmcd commented Jan 12, 2021

savitaashture left a comment

tekton-robot commented Jan 12, 2021

savitaashture left a comment

MarcelMue left a comment

MarcelMue Jan 13, 2021

dibyom Jan 13, 2021

MarcelMue Jan 13, 2021

dibyom Jan 13, 2021

dibyom Jan 13, 2021

MarcelMue Jan 13, 2021

dibyom Jan 13, 2021

tekton-robot commented Jan 13, 2021

MarcelMue commented Jan 14, 2021

tekton-robot commented Jan 14, 2021

savitaashture commented Jan 14, 2021

khrm left a comment

khrm commented Jan 15, 2021

tekton-robot commented Jan 15, 2021

tekton-robot commented Jan 15, 2021

tekton-robot commented Jan 15, 2021

MarcelMue commented Jan 15, 2021

Refactor sink test #882

Refactor sink test #882

Conversation

dibyom commented Dec 30, 2020 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Dec 30, 2020

tekton-robot commented Dec 30, 2020

tekton-robot commented Jan 6, 2021

tekton-robot commented Jan 6, 2021

tekton-robot commented Jan 7, 2021

bigkevmcd commented Jan 12, 2021

savitaashture left a comment

Choose a reason for hiding this comment

tekton-robot commented Jan 12, 2021

savitaashture left a comment

Choose a reason for hiding this comment

MarcelMue left a comment

Choose a reason for hiding this comment

MarcelMue Jan 13, 2021

Choose a reason for hiding this comment

dibyom Jan 13, 2021

Choose a reason for hiding this comment

MarcelMue Jan 13, 2021

Choose a reason for hiding this comment

dibyom Jan 13, 2021

Choose a reason for hiding this comment

dibyom Jan 13, 2021

Choose a reason for hiding this comment

MarcelMue Jan 13, 2021

Choose a reason for hiding this comment

dibyom Jan 13, 2021

Choose a reason for hiding this comment

tekton-robot commented Jan 13, 2021

MarcelMue commented Jan 14, 2021

tekton-robot commented Jan 14, 2021

savitaashture commented Jan 14, 2021

khrm left a comment

Choose a reason for hiding this comment

khrm commented Jan 15, 2021

tekton-robot commented Jan 15, 2021

tekton-robot commented Jan 15, 2021

tekton-robot commented Jan 15, 2021

MarcelMue commented Jan 15, 2021

dibyom commented Dec 30, 2020 •

edited

Loading