Deprecate and remove test builders

[imjasonh]

https://github.com/tektoncd/pipeline/tree/master/internal/builder

New uses of and additions to these packages should be discouraged, and eventually these packages should be phased out entirely.

Hand-maintaining these test builder methods is toilsome and error-prone. They claim to help readability, but instead they introduce a Tekton-specific dialect of Go when users could just write Go-standard struct declarations themselves, which have the advantage of named fields over positional function args.

See #3124 for an example of migrating off of test builders.

/assign @imjasonh

[imjasonh]

/kind cleanup

[pritidesai]

It will be great to break this down one test file or one Go struct (Task/PipelineTask/Pipeline) at a time and would make a good good-first-issue, see the changes here for reference.

[imjasonh]

Here's a little oneliner to find the top 10 files by occurrences of tb., which is generally what we alias test builder imports to:

$ git grep -c "tb\." | sed -e 's/:/ /g' | sort -k2 -n -r | head -n 10
pkg/reconciler/pipelinerun/pipelinerun_test.go 749
pkg/reconciler/taskrun/taskrun_test.go 737
pkg/reconciler/pipelinerun/resources/apply_test.go 170
pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go 128
internal/builder/v1beta1/task_test.go 83
internal/builder/v1beta1/pipeline_test.go 80
test/v1alpha1/pipelinerun_test.go 78
pkg/reconciler/events/cloudevent/cloud_event_controller_test.go 78
pkg/timeout/handler_test.go 74
pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go 64

And a count of total tb\. instances:

$ git grep "tb\." | wc -l
    3141

I'd probably start with just replacing references to tb.Pipeline and tb.PipelineRun, and, if you feel like it in the process, cleaning up instances where a test creates a Pipeline then a PipelineRun instead of just creating a PipelineRun with PipelineSpec defined inline directly (same with Tasks). That'd knock out a lot of instances pretty quickly. Checkpoint every so often and send a PR.

With enough of those changes you'd be able to delete some unused test builder methods, until eventually you have all of them and you can delete whole files/packages 🎉

(We can probably also delete internal/builder/**/*_test.go, which are just unit tests that check the builders work as intended, which is exactly the kind of hand-written code we don't need to be managing 😄 )

[pritidesai]

pkg/reconciler/pipelinerun/pipelinerun_test.go 749
pkg/reconciler/taskrun/taskrun_test.go 737

😱

pipelinerun_test.go and taskrun_test.go are two big beasts, this is a great start and will simplify them 👍
And both would need more refactoring as per the discussion here.

[imjasonh]

I've found a truly disgusting mis-feature of test builders! 😱

Pop quiz: What does this method do?

tb.PipelineTaskParam("foo", "bar", "baz", "hello")

Hint: godoc for PipelineTaskParam

Pencils down! It results in an object equivalent to:

v1beta1.Param{
  Name:  "foo",
  Value: *v1beta1.NewArrayOrString("bar", "baz", "hello"),
}

That's right! The first arg is the parameter name, and subsequent values are the parameter values -- possibly-array-typed, if there are 2+ variadic values given.

The raw Go struct is unarguably more verbose to write and read, but that's because it names the fields it's filling and makes it clearer what each string value is stating.

[imjasonh]

Another one-liner to find most used test builder methods:

$ git grep -o "func [A-Z][^(]*" internal/  | cut -d' ' -f2 | sort | uniq | xargs -n 1 -I{} git grep -o "tb\.{}(" | cut -d: -f2  |  sort  | uniq -c  | sort -n
...
  77 tb.PipelineResource(
  95 tb.PipelineRunSpec(
  95 tb.TaskRunLabel(
 104 tb.PipelineRun(
 104 tb.VolumeMount(
 107 tb.Task(
 107 tb.TaskRunNamespace(
 112 tb.TaskRunTaskRef(
 119 tb.PipelineSpec(
 123 tb.Pipeline(
 124 tb.TaskRunSpec(
 138 tb.PipelineResourceSpecParam(
 154 tb.TaskRun(
 164 tb.PipelineTask(

And one to find unused test builder methods we can probably just delete any time:

$ git grep -o "func [A-Z][^(]*" internal/  | cut -d' ' -f2 | sort | uniq | xargs -n 1 -I{} sh -c "git grep -o "tb\.{}" > /dev/null || echo  {}"  | grep -v "Example" | grep -v "Test"
ConditionAnnotations
ConditionDescription
ConditionLabels
ConditionParamSpec
ConditionResource
ConditionSpecCheckScript
PipelineRunAffinity
PipelineRunTolerations
PodCreationTimestamp
PodStatus
PodStatusConditions
StepCPU
StepEphemeralStorage
StepLimits
StepMemory
StepRequests
StepResources
StepTerminationMessagePath
TaskResultsOutput
TaskRunAffinity
TaskRunInputsParam
TaskRunPodSecurityContext
TaskRunTolerations

[vdemeester]

I've found a truly disgusting mis-feature of test builders! scream

Pop quiz: What does this method do?

tb.PipelineTaskParam("foo", "bar", "baz", "hello")

Hint: godoc for PipelineTaskParam

Pencils down! It results in an object equivalent to:

v1beta1.Param{
  Name:  "foo",
  Value: *v1beta1.NewArrayOrString("bar", "baz", "hello"),
}

That's right! The first arg is the parameter name, and subsequent values are the parameter values -- possibly-array-typed, if there are 2+ variadic values given.

Note that this is an API design (the builder API here), we could have done it differently and not have that confusing method 🙃

[imjasonh]

Note that this is an API design (the builder API here), we could have done it differently and not have that confusing method 🙃

Oh definitely, the builder could have been something like:

tb.PipelineTaskParam("foo",
  tb.PipelineTaskParamValues("bar", "baz", "hello"))

...and if we wanted to keep the builders we could refactor it to that already.

But is it more readable than the raw struct declaration, and is it more readable by enough to warrant us maintaining a method to make it possible, when the alternative is already available to us for free? I think we've decided the answer is no 😄

[vdemeester]

@imjasonh yeah agreed, I was just pointing out it's not the "builder" pattern that is wrong "by default" 😛

[popcor255]

/assign

[tekton-robot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

[tekton-robot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

[vdemeester]

/lifecycle frozen
Still work to do here.

[abayer]

/assign @abayer

[abayer]

/close

[tekton-robot]

@abayer: Closing this issue.

In response to this:

/close

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.