Simplify variable substitution tests ⚗️ #1702
Conversation
googlebot
added
the
cla: yes
tekton-robot
added
the
size/M
bobcatfish
force-pushed
the
whoops_deprecated
branch
from
62cf103
to
74398e3
Compare
tekton-robot
added
size/L
bobcatfish
changed the title
| Name: "$(inputs.params.FOO)", | ||
| }, | ||
| }, | ||
| }}, { |
There was a problem hiding this comment.
nit: indentation might be off here, I'd expect a }, { at the same indentation as Volumes: above.
| ClaimName: "$(inputs.params.FOO)", | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
same nit here: equally-indented }s just look wrong.
| Name: "somethingelse", | ||
| Default: builder.ArrayOrString(""), | ||
| }} | ||
| want := applyMutation(simpleTaskSpec, func(spec *v1alpha1.TaskSpec) { |
There was a problem hiding this comment.
applyMutation is grooooossss and we should get rid of it. But that's not for this PR.
There was a problem hiding this comment.
haha its very "clever"
bobcatfish
force-pushed
the
whoops_deprecated
branch
from
74398e3
to
5197987
Compare
|
I think I fixed the indentation, PTAL @vdemeester @imjasonh ! Use Golang they said... No one will ever have to argue about formatting again they said... |
Use any editor that can do |
|
[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:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
Each test case for parameter substitution application was being given a totally separate test case, with the variables being used being declared in different places across the file. For tektoncd#1639 I came along and wanted to start adding more tests for workspace substitution and found it hard to tell where to start so I: * Combined most of the test cases for param subsitution into one test so you can easily see everything that is being tested (none of the test cases conflicted with each other and can easily be applied together) * I kept the array param test cases separate cuz they seemd to be testing distinct test cases * The Volume test cases were a bit odd b/c they were trying to make sure substitution was _applied_ to volumes, but there is no volume specific function so they were calling an internal function and passing in dummy values that are not representative of the actual values you'd substitute for volumes so instead I folded these test cases into the param application test. Probably the resource application test case should be made quite similar to the param test but it seemed like some of the resource stuff was distinct and had to be tested in isolateion (e.g. just outputs, just inputs, etc.) Also removed some depreated (and duplicated!) volume tests: in tektoncd#1311 I removed support for ${} but instead of removing these tests I just updated them, making them duplicates of the above test cases.
bobcatfish
force-pushed
the
whoops_deprecated
branch
from
5197987
to
8d066e8
Compare
Changes
Each test case for parameter substitution application was being given a
totally separate test case, with the variables being used being declared
in different places across the file. For #1639 I came along and wanted
to start adding more tests for workspace substitution and found it hard
to tell where to start so I:
so you can easily see everything that is being tested (none of the
test cases conflicted with each other and can easily be applied
together)
testing distinct test cases
substitution was applied to volumes, but there is no volume specific
function so they were calling an internal function and passing in
dummy values that are not representative of the actual values you'd
substitute for volumes so instead I folded these test cases into the
param application test.
Probably the resource application test case should be made quite similar
to the param test but it seemed like some of the resource stuff was
distinct and had to be tested in isolateion (e.g. just outputs, just
inputs, etc.)
Also removed some depreated (and duplicated!) volume tests:
in #1311 I removed support for ${} but instead of removing these tests
I just updated them, making them duplicates of the above test cases.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe 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.