add custom parameters to structured tests #6055

JacekDuszenko · 2021-06-22T14:16:02Z

Description
Custom parameters can now be added to structured tests in form of yaml config list.

Custom parameters can now be added to structured tests in form of yaml config list.

codecov · 2021-06-22T14:33:52Z

Codecov Report

Merging #6055 (e1691e8) into master (30fded0) will decrease coverage by 0.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6055      +/-   ##
==========================================
- Coverage   70.55%   70.32%   -0.24%     
==========================================
  Files         468      471       +3     
  Lines       18036    18082      +46     
==========================================
- Hits        12726    12716      -10     
- Misses       4375     4436      +61     
+ Partials      935      930       -5

Impacted Files	Coverage Δ
pkg/skaffold/schema/latest/v1/config.go	`58.82% <ø> (ø)`
pkg/skaffold/test/structure/structure.go	`91.11% <100.00%> (+0.41%)`	⬆️
pkg/skaffold/deploy/deploy_mux.go	`61.70% <0.00%> (-4.21%)`	⬇️
pkg/skaffold/deploy/resource/deployment.go	`85.36% <0.00%> (-3.62%)`	⬇️
pkg/skaffold/runner/v1/deploy.go	`49.39% <0.00%> (-3.61%)`	⬇️
pkg/skaffold/runner/runcontext/context.go	`83.21% <0.00%> (-1.40%)`	⬇️
pkg/skaffold/event/event.go	`91.12% <0.00%> (-0.85%)`	⬇️
pkg/skaffold/deploy/kustomize/kustomize.go	`74.66% <0.00%> (-0.34%)`	⬇️
pkg/skaffold/deploy/helm/deploy.go	`78.45% <0.00%> (-0.24%)`	⬇️
pkg/skaffold/deploy/kpt/kpt.go	`81.12% <0.00%> (-0.21%)`	⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30fded0...e1691e8. Read the comment docs.

briandealwis

Thanks for your contribution! A few small changes.

briandealwis · 2021-06-22T18:10:04Z

pkg/skaffold/schema/latest/v1/config.go

@@ -491,6 +491,10 @@ type TestCase struct {
 	// to run on that artifact.
 	// For example: `["./test/*"]`.
 	StructureTests []string `yaml:"structureTests,omitempty" skaffold:"filepath"`
+
+	// StructureTestArgs lists additional configuration arguments passed to `container-structure-test` binary.
+	// For example: `["--driver tar", "--no-color", "-q"]`.


As I mention elsewhere in this review, we don't want to do arg parsing. But most long-form arguments support using an = so we should be able to use:

Suggested change

// For example: `["--driver tar", "--no-color", "-q"]`.

// For example: `["--driver=tar", "--no-color", "-q"]`.

briandealwis · 2021-06-22T18:10:50Z

pkg/skaffold/test/structure/structure.go

@@ -86,7 +89,10 @@ func (cst *Runner) runStructureTests(ctx context.Context, out io.Writer, imageTa
 	for _, f := range files {
 		args = append(args, "--config", f)
 	}
-
+	for _, customArg := range cst.structureTestArgs {
+		splittedCustomArg := strings.Fields(customArg)


We require the user to be responsible for splitting arguments properly. Otherwise we need to deal with quoting issues, and YAML already makes that hard enough. Docker uses this approach too.

briandealwis · 2021-06-22T18:14:58Z

pkg/skaffold/test/structure/structure_test.go

@@ -94,6 +94,44 @@ func TestIgnoreDockerNotFound(t *testing.T) {
 	})
 }

+func TestCustomParams(t *testing.T) {
+	const (
+		imageName = "image:tag"


Just inline this: you're using the string inline anyways further below.

briandealwis · 2021-06-22T18:18:35Z

pkg/skaffold/test/structure/structure.go

+		workspace:         tc.Workspace,
+		localDaemon:       localDaemon,
+		imageIsLocal:      imageIsLocal,
+		structureTestArgs: tc.StructureTestArgs,


I think it might be better to move this up below line 51 with the structureTests to keep the parameters together. localDaemon and imageIsLocal are internal matters.

briandealwis · 2021-06-22T18:19:59Z

pkg/skaffold/test/structure/structure_test.go

+		expectedBaseCmd := "container-structure-test test -v warn --image " + imageName + " --config " + tmpDir.Path("test.yaml")
+		t.Override(&util.DefaultExecCommand, testutil.CmdRun(expectedBaseCmd+" --driver tar --force --no-color -q --save"))
+
+		structureTestArgs := []string{"--driver tar", "--force", "--no-color", "-q", "--save"}


Suggested change

expectedBaseCmd := "container-structure-test test -v warn --image " + imageName + " --config " + tmpDir.Path("test.yaml")

t.Override(&util.DefaultExecCommand, testutil.CmdRun(expectedBaseCmd+" --driver tar --force --no-color -q --save"))

structureTestArgs := []string{"--driver tar", "--force", "--no-color", "-q", "--save"}

expectedBaseCmd := "container-structure-test test -v warn --image image:tag --config " + tmpDir.Path("test.yaml")

t.Override(&util.DefaultExecCommand, testutil.CmdRun(expectedBaseCmd+" --driver=tar --force --no-color -q --save"))

structureTestArgs := []string{"--driver=tar", "--force", "--no-color", "-q", "--save"}

briandealwis · 2021-06-22T18:21:38Z

pkg/skaffold/test/structure/structure_test.go

+		imageName = "image:tag"
+	)
+
+	testutil.Run(t, "", func(t *testutil.T) {


It would be useful to have tests here for: nil and an empty array too.

JacekDuszenko · 2021-06-22T18:50:50Z

@briandealwis Thanks for your comments! I'll address them shortly and submit an updated PR.

JacekDuszenko · 2021-06-22T20:05:20Z

@briandealwis I added a new commit addressing your remarks, feel free to re-review if you're free. Thanks

briandealwis

This looks great — thanks!

briandealwis · 2021-06-23T21:53:10Z

docs/content/en/samples/testers/structure/structureTestArgs.yaml

+      - --driver=tar
+      - -q
+      - --no-color
+      -  --test-report=TEST_REPORT_NAME


Suggested change

- --test-report=TEST_REPORT_NAME

- --test-report=TEST_REPORT_NAME

add custom parameters to structured tests

0f769a8

Custom parameters can now be added to structured tests in form of yaml config list.

JacekDuszenko requested a review from a team as a code owner June 22, 2021 14:16

JacekDuszenko requested a review from gsquared94 June 22, 2021 14:16

google-cla bot added the cla: yes label Jun 22, 2021

pull-request-size bot added the size/M label Jun 22, 2021

JacekDuszenko mentioned this pull request Jun 22, 2021

Support container-structure-test driver flag #5817

Closed

update docs

5bddb9b

briandealwis reviewed Jun 22, 2021

View reviewed changes

address review remarks

c2f6a65

pull-request-size bot added size/L and removed size/M labels Jun 22, 2021

move structureTestArgs in runner up

e1691e8

tejal29 added the kokoro:run runs the kokoro jobs on a PR label Jun 23, 2021

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jun 23, 2021

tejal29 assigned briandealwis and gsquared94 Jun 23, 2021

briandealwis approved these changes Jun 23, 2021

View reviewed changes

briandealwis merged commit c326595 into GoogleContainerTools:master Jun 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add custom parameters to structured tests #6055

add custom parameters to structured tests #6055

JacekDuszenko commented Jun 22, 2021

codecov bot commented Jun 22, 2021 •

edited

Loading

briandealwis left a comment

briandealwis Jun 22, 2021

briandealwis Jun 22, 2021

briandealwis Jun 22, 2021

briandealwis Jun 22, 2021

briandealwis Jun 22, 2021

briandealwis Jun 22, 2021

JacekDuszenko commented Jun 22, 2021

JacekDuszenko commented Jun 22, 2021

briandealwis left a comment

briandealwis Jun 23, 2021

	// For example: `["--driver tar", "--no-color", "-q"]`.
	// For example: `["--driver=tar", "--no-color", "-q"]`.

	- --test-report=TEST_REPORT_NAME
	- --test-report=TEST_REPORT_NAME

add custom parameters to structured tests #6055

add custom parameters to structured tests #6055

Conversation

JacekDuszenko commented Jun 22, 2021

codecov bot commented Jun 22, 2021 • edited Loading

Codecov Report

briandealwis left a comment

Choose a reason for hiding this comment

briandealwis Jun 22, 2021

Choose a reason for hiding this comment

briandealwis Jun 22, 2021

Choose a reason for hiding this comment

briandealwis Jun 22, 2021

Choose a reason for hiding this comment

briandealwis Jun 22, 2021

Choose a reason for hiding this comment

briandealwis Jun 22, 2021

Choose a reason for hiding this comment

briandealwis Jun 22, 2021

Choose a reason for hiding this comment

JacekDuszenko commented Jun 22, 2021

JacekDuszenko commented Jun 22, 2021

briandealwis left a comment

Choose a reason for hiding this comment

briandealwis Jun 23, 2021

Choose a reason for hiding this comment

codecov bot commented Jun 22, 2021 •

edited

Loading