AmitKumarDas / metac

It is metacontroller and more
Apache License 2.0
57 stars 16 forks source link

fix(integration-test): #136 fix test framework to execute tests #138

Closed AmitKumarDas closed 4 years ago

AmitKumarDas commented 4 years ago

This commit fixes the issue of integration tests that were never run.

Reason for this bug?

This has mostly happened when apiserver & etcd binaries were upgraded. The new binaries make use of new flags & deprecates use of old as well as insecure flags. The framework package present in test/integration did not handle these new changes and was rather swallowing the startup errors resulting in passing these tests that never ran.

What has been done?

The changes include copying of integration test related framework code from controller runtime library. The copied codebase fits the current integration test framework of Metac nicely. This approach also avoid import of un-required dependencies & associated go vendoring problems that often arises due to import of controller-runtime libraries.

closes https://github.com/AmitKumarDas/metac/issues/136

Refer - https://github.com/kubernetes-sigs/controller-runtime/tree/master/pkg/internal/testing

Signed-off-by: AmitKumarDas amit.das@mayadata.io

grzesuav commented 4 years ago

@AmitKumarDas there are two errors logged from integration tests :

--- PASS: TestInstallUninstallCRD (2.54s)
##[error]    install_uninstall_crd_test.go:341: CRD storages.dao.amitd.io should have been deleted: IsFinalized true

and

--- PASS: TestSetStatusOnCR (5.55s)
##[error]    set_status_on_cr_test.go:153: Waiting for verification of CoolNerd resource status

they are expected to be there ?

AmitKumarDas commented 4 years ago

@grzesuav the tests should pass eventually. Since the controller reconcile is eventual. I will still have a look once infront of my laptop.

AmitKumarDas commented 4 years ago

@grzesuav on further debugging, I found that those two errors were actually info messages that were logged via t.Log i.e. testing.T instance. I guess these were being logged as stderr. I have removed one of the logs since it was no more required. The other log uses klog.Infof now instead of t.Log.

grzesuav commented 4 years ago

I have approved but does not have any valuable input, so other approval would be appreciated

floriankoch commented 4 years ago

@AmitKumarDas i don't know the codebsae good enough to do a propper review

From the 10000 Feet View, the changes are looking good

AmitKumarDas commented 4 years ago

:tada: This PR is included in version 0.2.4 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: