Container for running tests and Makefile cleanup #385

kadel · 2017-01-20T16:30:07Z

This adds Dockerfile that can be used to run all Kompose tests locally.

make test will run exactly the same tests as travis-ci but running locally in docker image.
This can be used to tests everything before commiting or submitting PR.
Image is automatically build in docker hub - https://hub.docker.com/r/kompose/tests/ or it can be build locally using make test-image.

Automatic build at docker hub is NOT triggered automatically on push. Only time when rebuild is needed is when Dockerfile is changed.

This PR also includes Makefile and /script/ cleanup.
I've removed all the shell scripts that were just one single command and put those commands directly to Makefile. It is much more easier to figure out what is doing what.

cdrage · 2017-01-20T19:24:12Z

script/test_in_container/Dockerfile

@@ -0,0 +1,32 @@
+# This Dockefile creates image where all Kompose tests can be run
+
+FROM registry.centos.org/centos/centos:7


i think we should use the docker hub centos base images

I was thinking about it. They should be build from same Dockerfile.
I don't know.

Why would you prefer image from docker hub?

cdrage

LGTM 👍 other than my small changes.

This will make it 100% easier to replicate any tests that need to be ran!

cdrage · 2017-01-20T19:24:29Z

script/test_in_container/Dockerfile

@@ -0,0 +1,32 @@
+# This Dockefile creates image where all Kompose tests can be run


missing license at the top

fixed, thx for noticing

cdrage · 2017-01-20T19:25:01Z

script/test_in_container/Dockerfile

+# This Dockefile creates image where all Kompose tests can be run
+
+FROM registry.centos.org/centos/centos:7
+MAINTAINER Tomas Kral <tkral@redhat.com>


MAINTAINER has been deprecated in 1.13 of Docker :(.

Better to use LABEL maintainer Tomas Kral <tkral@redhat.com> or remove it all-together

Oh, I didn't know that.
I'll remove it, I don't think that it is something that we need for image. It was just something that I was used to doing.

cdrage · 2017-01-20T19:32:50Z

Just a note though, I feel as though these tests run a lot slower than the original ones? I think it's because there's no cache being passed through?

The part that's reallyyyy taking it's time is here:

# First install packages that are dependencies of the test. 
go test -i -race -cover ./cmd/... ./pkg/... .

Another being that what if I wanted to just run cmd or just run integration?

kadel · 2017-01-23T10:45:06Z

Just a note though, I feel as though these tests run a lot slower than the original ones? I think it's because there's no cache being passed through?

Yes, no cache being passed through.
I'll look at it in separate PR.

The part that's reallyyyy taking it's time is here:
# First install packages that are dependencies of the test. 
go test -i -race -cover ./cmd/... ./pkg/... .

This pre-compiles all the dependencies for test. So it can take some time, but it will speed up running all the go test.
Calculating coverage for whole project requires running go test separately for every package. If you do it without running go test -i first it takes really long time (20+mins) :-D.

Another being that what if I wanted to just run cmd or just run integration?

Yes, I was thinking about it. I would like to do it in separate PR.

I have some other ideas how to speed this up. But than it will run slightly different commands and tests from what is run in travis-ci. I wanted to replicated same thing that travis is doing first, and than add options to run faster but not complete test later.

If you didn't setup user.email and user.name `git commit` fails. This configures user.email and user.name for newly created temporary git repo.

If command is simple command call it from Makefile, there is no need to have them in separate shell scripts.

cdrage · 2017-01-23T13:28:33Z

I like how you keep the current tests in as well. But perhaps we can do it so that it's "test-container" or something?

Would be easier for using it on travis, etc and for those who choose to run the container vs traditional.

kadel · 2017-01-23T13:38:47Z

There is make test-all that runs all the tests outside container and make test that runs it inside container.
My idea was that "humans" will run tests in container so i wanted to make command shorter, hence make test :-)

So you are suggesting changing it so make test runs everything without container and creating make test-container that doesn't what make test is doing right now (running it it all inside container)?

cdrage · 2017-01-23T13:45:41Z

@kadel
Yup. That's what I'm suggesting. make test runs it traditionally. make test-container runs it in a container.

kadel · 2017-01-23T14:08:58Z

@kadel
Yup. That's what I'm suggesting. make test runs it traditionally. make test-container runs it in a container.

done ;-)

There is just one small catch with make test and that it expects that you have $GOPATH/bin in $PATH.
Otherwise test-cmd might fail or run against older binary.

cdrage

LGTM!

cdrage · 2017-01-23T14:11:55Z

Makefile

+# run all test localy in docker image (image can be build by by build-test-image target)
+.PHONY: test-container
+test-container:
+	docker run -v `pwd`:/opt/tmp/kompose:ro -it $(TEST_IMAGE) 


Thanks for the change!

kadel added kind/testing review needed labels Jan 20, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 20, 2017

kadel force-pushed the test-in-docker branch from 70a2225 to 81e985c Compare January 20, 2017 16:30

cdrage reviewed Jan 20, 2017

View reviewed changes

cdrage requested changes Jan 20, 2017

View reviewed changes

kadel force-pushed the test-in-docker branch from 81e985c to fbf690d Compare January 23, 2017 10:49

kadel added 4 commits January 23, 2017 11:49

Dockerfile for running tests

c8c2e8b

Add make rules for building and running test image

f68be75

Fix CreateLocalGitDirectory for tests.

f530f99

If you didn't setup user.email and user.name `git commit` fails. This configures user.email and user.name for newly created temporary git repo.

Clean up Makefile and scripts in scripts dir.

20b844d

If command is simple command call it from Makefile, there is no need to have them in separate shell scripts.

kadel force-pushed the test-in-docker branch from fbf690d to 20b844d Compare January 23, 2017 10:49

Rename some make targests: test->test-container test-all->test

0b1b7f3

kadel force-pushed the test-in-docker branch from 1b24691 to 0b1b7f3 Compare January 23, 2017 14:06

cdrage approved these changes Jan 23, 2017

View reviewed changes

kadel merged commit 001e19c into kubernetes:master Jan 24, 2017

kadel deleted the test-in-docker branch January 24, 2017 11:18

kadel restored the test-in-docker branch January 25, 2017 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Container for running tests and Makefile cleanup #385

Container for running tests and Makefile cleanup #385

kadel commented Jan 20, 2017

cdrage Jan 20, 2017

kadel Jan 23, 2017

cdrage left a comment

cdrage Jan 20, 2017

kadel Jan 23, 2017

cdrage Jan 20, 2017

kadel Jan 23, 2017

cdrage commented Jan 20, 2017

kadel commented Jan 23, 2017 •

edited

Loading

cdrage commented Jan 23, 2017 •

edited

Loading

kadel commented Jan 23, 2017

cdrage commented Jan 23, 2017

kadel commented Jan 23, 2017

cdrage left a comment

cdrage Jan 23, 2017 •

edited

Loading

		@@ -0,0 +1,32 @@
		# This Dockefile creates image where all Kompose tests can be run

		FROM registry.centos.org/centos/centos:7

Container for running tests and Makefile cleanup #385

Container for running tests and Makefile cleanup #385

Conversation

kadel commented Jan 20, 2017

cdrage Jan 20, 2017

Choose a reason for hiding this comment

kadel Jan 23, 2017

Choose a reason for hiding this comment

cdrage left a comment

Choose a reason for hiding this comment

cdrage Jan 20, 2017

Choose a reason for hiding this comment

kadel Jan 23, 2017

Choose a reason for hiding this comment

cdrage Jan 20, 2017

Choose a reason for hiding this comment

kadel Jan 23, 2017

Choose a reason for hiding this comment

cdrage commented Jan 20, 2017

kadel commented Jan 23, 2017 • edited Loading

cdrage commented Jan 23, 2017 • edited Loading

kadel commented Jan 23, 2017

cdrage commented Jan 23, 2017

kadel commented Jan 23, 2017

cdrage left a comment

Choose a reason for hiding this comment

cdrage Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

kadel commented Jan 23, 2017 •

edited

Loading

cdrage commented Jan 23, 2017 •

edited

Loading

cdrage Jan 23, 2017 •

edited

Loading