refactoring pipelinerun unit test #2876

pritidesai · 2020-06-29T23:55:59Z

Changes

Creating a common function reconcileRun to:

Create an instance of the PipelineRun controller - getPipelineRunController
Reconcile specified Data - PipelineRun, Pipeline, Tasks, etc.
Verify PipelineRun was reconciled
Check events introduced in PR Finish setting up events for pipeline runs #2874

All of the above steps were repeating in all the test functions 😢

Updated all unit tests in pipelinerun_test.go to utilize this common function.

Further possible refactoring:

Great suggestion from @afrittoli on using Matches instead of retrieving resources directly, here, which can go in a separate PR.
One more PR would be great to consolidate namespace instead of hardcoding foo everywhere.
One test failure results in metrics recorder failure for all unit tests and very hard to spot what was the actual cause of failure. Possibly fix the way we register metrics view.

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.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

NONE

tekton-robot · 2020-06-29T23:56:22Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/question: Issues or PRs that are questions around the project or a particular feature
kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

pritidesai · 2020-06-29T23:56:28Z

/kind cleanup

tekton-robot · 2020-06-29T23:56:34Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/question: Issues or PRs that are questions around the project or a particular feature
kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

pritidesai · 2020-06-29T23:57:17Z

pkg/reconciler/pipelinerun/pipelinerun_test.go

@@ -45,7 +45,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	k8stesting "k8s.io/client-go/testing"
 	ktesting "k8s.io/client-go/testing"


the same module was imported twice with different labels 😕

pritidesai · 2020-06-29T23:58:46Z

pkg/reconciler/pipelinerun/pipelinerun_test.go

-	if err != nil {
-		t.Fatalf("Somehow had error getting reconciled run out of fake client: %s", err)
+	if len(actions) < 2 {
+		t.Fatalf("Expected client to have at least two action implementation but it has %d", len(actions))


Have added check on length before accessing that index in the list

thanks for explaining! really helps when reviewing :D

and good call!!

pritidesai · 2020-07-06T18:54:24Z

@vdemeester I rebased this PR but the label (needs-rebase) wasn't removed, weird 🤔

pritidesai · 2020-07-07T17:05:58Z

/test pull-tekton-pipeline-integration-tests

pritidesai · 2020-07-20T20:55:26Z

Diff in this PR looks huge 🤦‍♀️. Its all about introducing a common function to build PipelineRun which is used by all unit tests in pipelinerun_test.go instead of repeating them in all the tests, would appreciate your review 🙏

vdemeester

SGTM
/cc @afrittoli @bobcatfish

ghost

Awesome, thank you!

tekton-robot · 2020-07-21T13:01:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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

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

bobcatfish · 2020-07-21T13:30:03Z

Diff in this PR looks huge 🤦‍♀️.

it's pretty amazing that it's size/XL and it's only one file! I'd love to find ways to have less tests in there one day 🙏 or maybe targeting the repetition is the way to go!! thanks for taking this on @pritidesai !

bobcatfish

This looks great! I have some feedback on how the code is organized over all (i.e.: you've made the code a lot better, here are some ways we can make it even better).

The only thing I'd want to see changed before merging this PR is the names of the new functions, to better reflect that they do more than just "get", other than that I'm happy to merge :D

Thanks again @pritidesai !!

bobcatfish · 2020-07-21T13:30:48Z

pkg/reconciler/pipelinerun/pipelinerun_test.go

-	if err != nil {
-		t.Fatalf("Somehow had error getting reconciled run out of fake client: %s", err)
+	if len(actions) < 2 {
+		t.Fatalf("Expected client to have at least two action implementation but it has %d", len(actions))


thanks for explaining! really helps when reviewing :D

and good call!!

bobcatfish · 2020-07-21T13:35:24Z

pkg/reconciler/pipelinerun/pipelinerun_test.go

+	rs []*v1alpha1.PipelineResource,
+	conditions []*v1alpha1.Condition,
+	namespace, prName string,
+	wantEvents []string,


minor: using newlines like this is not very standard in our Go codebase, we're much more likely to put these together on one line. ways to improve this:

Docstring

Long param lists are often identifed as a "code smell" (https://refactoring.guru/smells/long-parameter-list https://www.codingrevolution.com/long-parameter-list/#:~:text=Long%20parameter%20list%20in%20a,four%20is%20considered%20too%20many) the solution is usually to group these args together into an object you pass in - in this case I could see maybe instantiating a struct with these args and calling functions on it - that might help with some of the ambiguity around the many responsibilities of this function as well (if you wanted to break it up into multiple functions)

bobcatfish · 2020-07-21T13:37:36Z

pkg/reconciler/pipelinerun/pipelinerun_test.go

+	}
+}
+
+func getReconciledRun(


I think it's important to try to indicate with naming whether mutations / assertions are happening in functions - reading the name of getReconciledRun I would expect it to be only retrieving a run, but it seems to be doing several things:

instantiating a controller

reconciling

asserting against events

Some alternative naming ideas: reconcileRun (dropping "get" makes it seem less like it's just retrieving) assertRunSucceeds

Another idea, mentioned below as well: create a struct that holds all the args below and expose several functions instead of just one - this will probably still be significantly easier to read and work with than the amount of repetition we have today

renamed getReconciledRun to reconcileRun

Creating struct is a great suggestion, I created a struct PipelineRunTest and made reconcileRun as an attribute to PipelineRunTest.

bobcatfish · 2020-07-21T13:38:41Z

pkg/reconciler/pipelinerun/pipelinerun_test.go

+	recorder := testAssets.Recorder
+
+	if reconcileError := c.Reconciler.Reconcile(context.Background(), namespace+"/"+prName); reconcileError != nil {
+		t.Fatalf("Error reconciling: %s", reconcileError)


have you considered using t.Helper? (https://blog.golang.org/go1.9)

It might be worth comparing what it looks like when one of the tests using this helper fails using it vs not using it (it might make more sense if you were to break this function up into a few)

Yup, tried it, very useful, instead of pointing back at the common utility (reconcileRun), it points back where the failure is originated, in actual test function 👍

pritidesai · 2020-07-22T01:41:33Z

Based on @bobcatfish's comments, I have added these changes:

Renamed getReconciledRun to reconcileRun
added t.Helper to reconcileRun
Created a new struct with all the arguments which were passed to getReconciledRun and define this common function as an attribute to PipelineRunTest

func (prt PipelineRunTest) reconcileRun() (*v1beta1.PipelineRun, test.Clients) {

pritidesai · 2020-07-22T02:10:45Z

/test pull-tekton-pipeline-integration-tests

pritidesai · 2020-07-22T03:19:47Z

@bobcatfish its ready for review again, PTAL 😸

There could be more refactoring done, mainly the way namespace and pipelineRun name are hard coded in the functions.

tekton-robot · 2020-07-27T21:05:13Z

@pritidesai: PR needs rebase.

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.

bobcatfish

Thanks @pritidesai !! I described one step further that I think we can go that would make the PipelineRunTest interface just a little bit better, but im also happy to merge as is if you'd like (esp if you want to just get this improvement in so you can stop dealing with rebasing all the time!!)

bobcatfish · 2020-07-28T17:43:02Z

pkg/reconciler/pipelinerun/pipelinerun_test.go

+	}
+
+	// Check generated events match what's expected
+	if len(prt.WantEvents) > 0 {


this is fairly minor but something feels weird to me about having WantEvents and PRName be attributes of PipelineRunTest - i think it's that this makes it so that if you want to call reconcileRun more than once in the same test, e.g. for two different pipelineruns, one reconciled after the other, you'd need to modify PipelineRunTest

I could see making 2 changes:

Make WantEvents and PRName and Namespace arguments to this function

Have a NewPipelineRunTest function that returns an instance of PipelineRunTest and takes Test and Data as arguments

this way you can call NewPipelineRunTest to create your controller with the state it needs (store the testAssets as an attribute of PipelineRunTest) and you can call reconcileRun with as many runs as you want

Hey @bobcatfish, I have added NewPipelineRunTest and updated reconcileRun by adding function parameters (Namespace, PipelineRunName, wantEvents, and permanentError).

Now, the PipelineRunTest has:

test.Data `json:"inline"` Test *testing.T TestAssets test.Assets Cancel func()

Also, have added defer on Cancel as part of each unit test instead of making it part of common function.

Data can be dropped from the PipelineRunTest struct since NewPipelineRunTest is creating the controller and instantiating PipelineRunTest with assets, data is not used/needed anywhere beyond NewPipelineRunTest - But I think it makes PipelineRunTest complete (with inputs and assets), can create separate PR to drop it if needed 😉

creating a common function to build reconciledRun and updating all tests to use that function wherever possible

bobcatfish

Thanks for cleaning this up and tolerating all the back and forth @pritidesai !

/lgtm

bobcatfish · 2020-07-29T15:06:41Z

pkg/reconciler/pipelinerun/pipelinerun_test.go

+
+// NewPipelineRunTest returns PipelineRunTest with a new PipelineRun controller created with specified state through data
+// This PipelineRunTest can be reused for multiple PipelineRuns by calling reconcileRun for each pipelineRun
+func NewPipelineRunTest(data test.Data, t *testing.T) *PipelineRunTest {


i'm now just nitpicking, but i'd put the functions that are related to PipelineRunTest near the declaration of the struct. dont wanna block the PR tho!

thanks @bobcatfish I was planning on moving all helper functions including:

PipelineRunTest struct declaration and related functions

getPipelineRunController

conditionCheckFromTaskRun

checkEvents

eventFromChannel

etc, to a separate file /pkg/reconciler/pipelinerun/pipelinerun_helpers_test.go as a follow up PR. pkg/reconciler/pipelinerun/pipelinerun_test.go is a combination of helper utilities and actual unit tests and helper functions are spread across this file 😞 Will be easier to introduce more tests and/or reviewing them with such separation.

okay great!

A couple of thoughts:

Somewhere in the top level test directory might be a good place to go, since that is where we are already putting our test libs there (might even make sense to work it into https://github.com/tektoncd/pipeline/blob/master/test/controller.go)

I want to caution against naming and calling code "helpers" - this is totally a common practice but I think it's useful to get out of the habit, for a couple of reasons:

People tend to use this terminology about libs used for test code - sometimes they use it for the code under tests as well but i think that's less common - and it seems almost like it's putting the code down, i.e., "i need this code to help me" whereas I think that well factored test code is just as important as any other well factored code, so libraries for test code are just as important as other code < rant over >

The other (probably bigger and less my personal opinion) reason is that "helper" is not a very specific name - good naming and code organization requires thinking about what the code's responsibility is and naming and organizing it based on that; using names like "helper" "manager" "library" "util" is a short cut that prevents us from doing that. More on this at https://dave.cheney.net/2019/01/08/avoid-package-names-like-base-util-or-common and https://blog.golang.org/package-names in the section "bad package names":

Avoid meaningless package names. Packages named util, common, or misc provide clients with no sense of what the package contains. This makes it harder for clients to use the package and makes it harder for maintainers to keep the package focused. Over time, they accumulate dependencies that can make compilation significantly and unnecessarily slower, especially in large programs. And since such package names are generic, they are more likely to collide with other packages imported by client code, forcing clients to invent names to distinguish them.

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 29, 2020

tekton-robot requested review from imjasonh and vdemeester June 29, 2020 23:56

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

pritidesai commented Jun 29, 2020

View reviewed changes

pritidesai force-pushed the pipelinerun_test branch from 9e2766a to 18d55b3 Compare June 30, 2020 17:50

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 3, 2020

pritidesai force-pushed the pipelinerun_test branch from 18d55b3 to 8ee2753 Compare July 6, 2020 17:56

pritidesai force-pushed the pipelinerun_test branch from 8ee2753 to 4beb2e3 Compare July 6, 2020 23:21

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2020

pritidesai force-pushed the pipelinerun_test branch from 4beb2e3 to d90e9e2 Compare July 7, 2020 23:42

vdemeester reviewed Jul 21, 2020

View reviewed changes

tekton-robot requested review from afrittoli and bobcatfish July 21, 2020 10:11

ghost approved these changes Jul 21, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2020

bobcatfish reviewed Jul 21, 2020

View reviewed changes

pritidesai force-pushed the pipelinerun_test branch from d90e9e2 to 0361e49 Compare July 22, 2020 01:32

pritidesai force-pushed the pipelinerun_test branch from 0361e49 to 514da32 Compare July 22, 2020 01:45

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2020

bobcatfish reviewed Jul 28, 2020

View reviewed changes

refactoring pipelinerun test

b57563c

creating a common function to build reconciledRun and updating all tests to use that function wherever possible

pritidesai force-pushed the pipelinerun_test branch from 514da32 to b57563c Compare July 29, 2020 00:41

vdemeester removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2020

bobcatfish reviewed Jul 29, 2020

View reviewed changes

tekton-robot assigned bobcatfish Jul 29, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2020

tekton-robot merged commit d1c770a into tektoncd:master Jul 29, 2020

pritidesai mentioned this pull request Jul 29, 2020

Run TestReconcileTimeouts as separate tests ⏰ #3030

Merged

3 tasks

pritidesai mentioned this pull request Sep 9, 2020

Deprecate and remove test builders #3178

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactoring pipelinerun unit test #2876

refactoring pipelinerun unit test #2876

pritidesai commented Jun 29, 2020 •

edited

Loading

tekton-robot commented Jun 29, 2020

pritidesai commented Jun 29, 2020

tekton-robot commented Jun 29, 2020

pritidesai Jun 29, 2020 •

edited

Loading

pritidesai Jun 29, 2020

bobcatfish Jul 21, 2020

pritidesai commented Jul 6, 2020

pritidesai commented Jul 7, 2020

pritidesai commented Jul 20, 2020

vdemeester left a comment

ghost left a comment

tekton-robot commented Jul 21, 2020

bobcatfish commented Jul 21, 2020

bobcatfish left a comment

bobcatfish Jul 21, 2020

bobcatfish Jul 21, 2020

bobcatfish Jul 21, 2020

pritidesai Jul 22, 2020

bobcatfish Jul 21, 2020

pritidesai Jul 22, 2020 •

edited

Loading

pritidesai commented Jul 22, 2020

pritidesai commented Jul 22, 2020

pritidesai commented Jul 22, 2020

tekton-robot commented Jul 27, 2020

bobcatfish left a comment

bobcatfish Jul 28, 2020

pritidesai Jul 29, 2020

pritidesai Jul 29, 2020 •

edited

Loading

bobcatfish left a comment

bobcatfish Jul 29, 2020

pritidesai Jul 29, 2020 •

edited

Loading

bobcatfish Jul 29, 2020

refactoring pipelinerun unit test #2876

refactoring pipelinerun unit test #2876

Conversation

pritidesai commented Jun 29, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Jun 29, 2020

pritidesai commented Jun 29, 2020

tekton-robot commented Jun 29, 2020

pritidesai Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pritidesai commented Jul 6, 2020

pritidesai commented Jul 7, 2020

pritidesai commented Jul 20, 2020

vdemeester left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

tekton-robot commented Jul 21, 2020

bobcatfish commented Jul 21, 2020

bobcatfish 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

pritidesai Jul 22, 2020 • edited Loading

Choose a reason for hiding this comment

pritidesai commented Jul 22, 2020

pritidesai commented Jul 22, 2020

pritidesai commented Jul 22, 2020

tekton-robot commented Jul 27, 2020

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pritidesai Jul 29, 2020 • edited Loading

Choose a reason for hiding this comment

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pritidesai Jul 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pritidesai commented Jun 29, 2020 •

edited

Loading

pritidesai Jun 29, 2020 •

edited

Loading

pritidesai Jul 22, 2020 •

edited

Loading

pritidesai Jul 29, 2020 •

edited

Loading

pritidesai Jul 29, 2020 •

edited

Loading