Local integration tests #256

bobcatfish · 2018-07-25T23:05:15Z

When starting to look into #251 I wanted to try to reproduce the test flakes locally but I immediately ran into two problems:

The tests hardcode a GCP project and bucket, which means I have to have access to those to run the tests.
The tests have to be triggered via integration_tests.sh to work, b/c it contains important setup, and this makes it a bit tough to run tests individually

(1) Can be resolved by adding me to these projects, but it turns out the tests can work just fine using a different project and bucket, so I've updated the tests such that these values can be provided as arguments to the tests.
(2) Now the setup (i.e. the creation of the build context) is done as part of the tests themselves, so we can run the tests directly with go test.

Bonuses:

The tests previously relied on TestRun being run before TestLayers because TestRun would actually build the images with docker and kaniko - this means running tests from TestLayers individually did not work. Now the tests will check if the images have been built before trying to use them, build them if they haven't been built, or use the previously built images if they have been.
There will now be no possibility of a collision if the tests are run simultaneously using the same GCS bucket b/c the context tarball name will include a UUID.

bobcatfish · 2018-07-25T23:05:38Z

kokoro is going to love this, I can just feel it

bobcatfish · 2018-07-25T23:26:11Z

o rly

bobcatfish · 2018-07-25T23:27:57Z

integration/cleanup.go:25:1:warning: runOnInterrupt is unused (deadcode)
integration/gcs.go:60:1:warning: deleteFromGCS is unused (deadcode)
integration/gcs.go:48:1:warning: uploadBuildContext is unused (deadcode)
integration/gcs.go:28:1:warning: createIntegrationTarball is unused (deadcode)
integration/images.go:72:1:warning: findDockerFiles is unused (deadcode)
integration/images.go:64:1:warning: getDockerImage is unused (deadcode)

wut

priyawadhwa

This looks really great, thanks for fixing all of this!

priyawadhwa · 2018-07-26T00:20:34Z

DEVELOPMENT.md

+
+#### Troubleshooting
+
+If you see errors due to `container-diff` failing, try pulling the base images (specified in [the


Just realized we could also get rid of this problem by using fully qualified images for all the Dockerfiles, so once this goes through hopefully we won't need this section!

ooooo nice! and it's in, so I'll remove this from the doc

priyawadhwa · 2018-07-26T00:26:46Z

DEVELOPMENT.md

+```shell
+export GCS_BUCKET="gs://<your bucket>"
+export IMAGE_REPO="gcr.io/somerepo"
+go test -v --bucket $GCS_BUCKET --repo $IMAGE_REPO -run TestLayers/test_layer_Dockerfile_test_copy_bucket


Since we have the flags here anyway, it might be easier to just pass in the names instead of having env variables as an extra step?

kk sounds good!

Okay so I'm going to do something halfway to what you are suggesting, just removing the export lines so it looks like this:

go test -v --bucket $GCS_BUCKET --repo $IMAGE_REPO -run TestLayers/test_layer_Dockerfile_test_copy_bucket`

One alternative would be to hardcode some defaults, but i think this makes it hard for ppl to know what they need to change:

go test -v --bucket gs://kaniko-test-bucket --repo gcr.io/kaniko-test -run TestLayers/test_layer_Dockerfile_test_copy_bucket`

Or we could do something like this, which is what I used to do all the time, but the thing is you can't copy and paste it:

go test -v --bucket <your bucket> --repo <your repo> -run TestLayers/test_layer_Dockerfile_test_copy_bucket`

I like leaving the env vars in b/c then if ppl use env vars they can copy & paste the command from the doc

For sure, SGTM!

priyawadhwa · 2018-07-26T00:44:17Z

integration/gcs.go

+	if err != nil {
+		return "", fmt.Errorf("Failed to create temporary directoy to hold tarball: %s", err)
+	}
+	uuid := randomString(32)


It might be simpler to use a timestamp here, since the context will be deleted anyway!

oh good call!

sharifelgamal · 2018-07-26T22:06:45Z

integration/integration_test.go

 				t.Error(err)
 				t.Fail()
 			}
 		})
 	}
 }

-func checkLayers(image1, image2 string, offset int) error {
+func checkLayers(t *testing.T, image1, image2 string, offset int) error {


Why do we need to pass t in here?

oh thanks for catching that @sharifelgamal I'll remove it - I had tried out calling t.Helper() in this function to change the line number reported for failures, but I ended up thinking it's better reporting the line number inside checkLayers

bobcatfish · 2018-07-27T22:42:53Z

PTAL @priyawadhwa @sharifelgamal

bobcatfish · 2018-07-27T23:16:45Z

😅

priyawadhwa · 2018-07-30T23:16:59Z

LGTM, I think you just need to rebase & this should be ready to merge!

bobcatfish · 2018-07-31T01:03:22Z

LGTM, I think you just need to rebase & this should be ready to merge!

HERE WE GO that flake better not happen, that would be ironic

<.<

dlorenc · 2018-07-31T15:25:06Z

integration/images.go:68:1:warning: singleSnapshotImages is unused (deadcode)
integration/images.go:29:1:warning: singleSnapshotFlag is unused (deadcode)

No flake, but a lint error!

bobcatfish · 2018-07-31T16:50:51Z

😥

To allow contributors to run the integration tests with their own GCS buckets and image repos (since not all contributors will have accesss to the projects used by the kaniko maintainers) this updates the integration tests so that these can be provided on the command line. This allows tests to be run individually, without using `make integration-test`. Previously, part of the test setup was done in the shell script (creating the context tarball that is required for the tests that build images with context). Instead it will be done in the test iself, so we can use `go test` to run tests individually if we want to. If we are running only one individual test, we don't want to build all of the images, so this commit creates a builder which tracks which images it has built and can be used by a tests to check if it should build an image before running, or it will use the images that have already been built by a previous test. The name of the context tarball has also been made unique (it includes the unix timestamp) to avoid potential test flakes if two tests using the same GCS bucket run simultaneously.

bobcatfish · 2018-07-31T16:54:19Z

7th time's a charm!

sharifelgamal

Awesome.

bobcatfish requested review from priyawadhwa and sharifelgamal July 25, 2018 23:05

bobcatfish force-pushed the local_integration_tests branch 2 times, most recently from ab753a6 to a7dc7b3 Compare July 25, 2018 23:51

priyawadhwa reviewed Jul 26, 2018

View reviewed changes

sharifelgamal reviewed Jul 26, 2018

View reviewed changes

bobcatfish force-pushed the local_integration_tests branch from a7dc7b3 to a7a357b Compare July 27, 2018 22:42

bobcatfish force-pushed the local_integration_tests branch from a7a357b to 395d012 Compare July 27, 2018 23:17

bobcatfish mentioned this pull request Jul 27, 2018

Number of layers inconsistent with kaniko builds that copy/copy the same files multiple times #251

Closed

bobcatfish force-pushed the local_integration_tests branch from 395d012 to 0c119fb Compare July 27, 2018 23:35

priyawadhwa approved these changes Jul 30, 2018

View reviewed changes

bobcatfish force-pushed the local_integration_tests branch from 0c119fb to eb91562 Compare July 31, 2018 01:01

bobcatfish force-pushed the local_integration_tests branch from eb91562 to b5a4d76 Compare July 31, 2018 16:54

sharifelgamal approved these changes Jul 31, 2018

View reviewed changes

bobcatfish merged commit 0d7eba9 into GoogleContainerTools:master Jul 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Local integration tests #256

Local integration tests #256

bobcatfish commented Jul 25, 2018

bobcatfish commented Jul 25, 2018

bobcatfish commented Jul 25, 2018

bobcatfish commented Jul 25, 2018 •

edited

Loading

priyawadhwa left a comment

priyawadhwa Jul 26, 2018

bobcatfish Jul 26, 2018

priyawadhwa Jul 26, 2018

bobcatfish Jul 26, 2018

bobcatfish Jul 27, 2018

priyawadhwa Jul 27, 2018

priyawadhwa Jul 26, 2018

bobcatfish Jul 26, 2018

sharifelgamal Jul 26, 2018

bobcatfish Jul 27, 2018

bobcatfish commented Jul 27, 2018

bobcatfish commented Jul 27, 2018

priyawadhwa commented Jul 30, 2018

bobcatfish commented Jul 31, 2018

dlorenc commented Jul 31, 2018

bobcatfish commented Jul 31, 2018

bobcatfish commented Jul 31, 2018

sharifelgamal left a comment


		#### Troubleshooting

		If you see errors due to `container-diff` failing, try pulling the base images (specified in [the

Local integration tests #256

Local integration tests #256

Conversation

bobcatfish commented Jul 25, 2018

bobcatfish commented Jul 25, 2018

bobcatfish commented Jul 25, 2018

bobcatfish commented Jul 25, 2018 • edited Loading

priyawadhwa 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

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

bobcatfish commented Jul 27, 2018

bobcatfish commented Jul 27, 2018

priyawadhwa commented Jul 30, 2018

bobcatfish commented Jul 31, 2018

dlorenc commented Jul 31, 2018

bobcatfish commented Jul 31, 2018

bobcatfish commented Jul 31, 2018

sharifelgamal left a comment

Choose a reason for hiding this comment

bobcatfish commented Jul 25, 2018 •

edited

Loading