Separate unit test and integration tests #15
Conversation
There was a problem hiding this comment.
i would align with the same framework then all operators here.
https://github.com/kubic-project/dex-operator/blob/master/pkg/test/util.go#L18
(including name of functions, structure and pkg)
| log.Print("Skipping tests in short mode") | ||
| } | ||
| os.Exit(0) | ||
| } |
There was a problem hiding this comment.
Shouldn't this resemble https://github.com/kubic-project/dex-operator/blob/master/pkg/test/util.go ?
There was a problem hiding this comment.
I think that for avoiding this duplication we could create a common operator tools where we put common things.
There was a problem hiding this comment.
i am ok for this common library shared among operators. It could be test but other stuff too @inercia thoughts about this?
There was a problem hiding this comment.
I agree we should use same names. Now, I find SkipIfShort better than SkipIfNotIntegration because the later asumes not short == integration. But there are other usages for short, like skipping long running tests.
| @@ -33,6 +33,12 @@ var cfg *rest.Config | |||
| var c client.Client | |||
|
|
|||
| func TestMain(m *testing.M) { | |||
|
|
|||
| if testing.Short() { | |||
| log.Print("Skippig integration tests in short mode") | |||
There was a problem hiding this comment.
Typo, Skipping, and double space.
|
|
||
| if testing.Short() { | ||
| log.Print("Skippig integration tests in short mode") | ||
| return |
There was a problem hiding this comment.
This return does not do what we expect. If you are in testing.Short() it will skip the whole test suite, and you just want to run this suite because unit tests could exist inside (https://github.com/kubic-project/dex-operator/blob/master/pkg/apis/kubic/v1beta1/v1beta1_suite_test.go#L38-L65)
|
|
||
| test.SkipSetupIfShort(m) | ||
|
|
||
| log.Print("Still here") |
| @@ -35,6 +36,11 @@ import ( | |||
| var cfg *rest.Config | |||
|
|
|||
| func TestMain(m *testing.M) { | |||
|
|
|||
| test.SkipSetupIfShort(m) | |||
There was a problem hiding this comment.
We should only avoid start the integration control plane in the suite test, but really execute m.Run so all tests that are not skipped within are actually executed.
Something like: https://github.com/kubic-project/dex-operator/blob/master/pkg/controller/dex/dex_controller_suite_test.go#L40-L60
| @@ -42,6 +43,9 @@ var depKey = types.NamespacedName{Name: "foo-deployment", Namespace: "default"} | |||
| const timeout = time.Second * 5 | |||
|
|
|||
| func TestReconcile(t *testing.T) { | |||
|
|
|||
| test.SkipTestIfShort(t) | |||
pkg/test/util.go
Outdated
| if testing.Short() { | ||
| log.Print("Skipping tests in short mode") | ||
| } | ||
| os.Exit(0) |
There was a problem hiding this comment.
Since m.Run() needs to be called, this os.Exit(0) will avoid that, and you could be skipping unit tests, because you won't reach m.Run() in TestMain in the suite test.
Aside from that, here we are always running os.Exit(0) :)
Makefile
Outdated
| @@ -167,6 +167,10 @@ check: $(GOLINT) | |||
|
|
|||
| .PHONY: test | |||
| test: | |||
| @$(GO) test -short -v $(SOURCES_DIRS_GO) -coverprofile cover.out | |||
There was a problem hiding this comment.
Minor: extra space before -v
| @@ -42,6 +43,9 @@ var depKey = types.NamespacedName{Name: "foo-deployment", Namespace: "default"} | |||
| const timeout = time.Second * 5 | |||
|
|
|||
| func TestReconcile(t *testing.T) { | |||
|
|
|||
| test.SkipUnlessIntegrationTesting(t) | |||
There was a problem hiding this comment.
This indentation is weird.
|
@pablochacin @ereslibre for the format things and syntax, for the golint and fmt we should check really and make them fail. otherwise in all operator we are keeping reviewing syntax/lint thing which is a pain, it should be done automagically for this |
|
There's a conflict with |
Yes, @pablochacin wants to make this changes in a different PR, I'm fine with that, once the conflict is resolved and commits squashed. |
pablochacin
force-pushed
the
separate-tests
branch
from
d5a059d
to
063bb9b
Compare
Skip tests that require environement sutup (e.g. k8s cluster) when running tests in short mode. Add an additional 'integarion' target to Makefile for running those tests. Signed-off-by: Pablo Chacin <pchacin@suse.com>
pablochacin
force-pushed
the
separate-tests
branch
from
063bb9b
to
2c8ced4
Compare
Skip tests that require environement sutup (e.g. k8s cluster)
when running tests in short mode. Add an additional 'integarion'
target to Makefile for running those tests.
Signed-off-by: Pablo Chacin pchacin@suse.com