Add initial test structure. #3
Conversation
mpereira
force-pushed
the
initial-test-structure
branch
3 times, most recently
from
1eff034
to
b8fe450
Compare
mpereira
force-pushed
the
initial-test-structure
branch
3 times, most recently
from
167fd0f
to
d5b9563
Compare
mpereira
force-pushed
the
initial-test-structure
branch
from
5a9bf76
to
80382ff
Compare
There was a problem hiding this comment.
Code looks great! You might want to ask Marcin to take a look at this PR as he's got a sharp eye for shell scripts and could offer better feedback wrt Golang/k8s!
Not too many comments my side. Locally though, I did run into 1 test failure myself when I gave the tests/run.sh.
- edit: nvm me, I'm dumb and didn't
k kudo initfirst though I had figured that would be taken care of based on this line
Anyhow, tests worked!
Ran 2 of 2 Specs in 106.554 seconds
SUCCESS! -- 2 Passed | 0 Failed | 0 Pending | 0 Skipped
PASS
But it's worth pointing out that default konvoy cluster size was not adequate to run tests. I had to add another 3 free nodes to account for the cassandra nodes cpu requirements as the konvoy addons take up a pretty big amount of space.
| &metav1.DeleteOptions{}, | ||
| ) | ||
| if err != nil { | ||
| log.Warnf("Error deleting namespace '%s': %s", namespaceName, err) |
There was a problem hiding this comment.
Might want to add retrying and make this a Fatal error. Not successfully deleting a namespace can result in subsequent test failures.
See kudo-spark PR d2iq-archive/kudo-spark-operator#32
There was a problem hiding this comment.
Instead of a fatal, we should return err and handle the error
tests should be failing with a clear reason and using log.Fatalf inside tests can lead to test terminate abruptly where it's hard to understand what happened. As we will only see that namespace deletion was failed. Instead of that t.Fatalf is preferred that will mark the test as failed. Or even better are the asserts that are being used in these tests.
But we don't need it here to obscure the code execution with fatals, and here its would be even simpler and understandable with
Expect(k8s.DeleteNamespace(TestNamespace)).To(BeNil())
On the other side of test structure re-using namespace shouldn't be the way to go for testing unless we are testing to re-use the namespace
There was a problem hiding this comment.
Agree with returning and handling error vs using fatal, from a library perspective it makes more sense imo.
Regarding re-using namespaces, my idea is to create namespaces based on the test module name, like in the simple-install-uninstall-test.
There was a problem hiding this comment.
@akirillov @rpalaznik, perhaps this approach should be done in kudo-spark?
There was a problem hiding this comment.
The general ideas for this testing structure are in the proposal document I shared earlier if you haven't seen it yet.
mpereira
force-pushed
the
initial-test-structure
branch
from
aff83c0
to
4f59002
Compare
mpereira
force-pushed
the
initial-test-structure
branch
from
4f59002
to
7d97188
Compare
mpereira
force-pushed
the
initial-test-structure
branch
from
01ad12e
to
ed54183
Compare
mpereira
force-pushed
the
initial-test-structure
branch
3 times, most recently
from
e9e1242
to
c9a0f68
Compare
mpereira
force-pushed
the
initial-test-structure
branch
from
c9a0f68
to
4bc0d1d
Compare
|
Check failed on CI cluster teardown flake. Merging. |
* Create initial testing structure and tests. * Build docker image. * Format with `goimports -w .`. * Get operator name from environment variable. * Script improvements. * Document command for building Docker images. * Add usage to uninstall_operator.sh script. * Add tooling to check formatting and format go code. * Update data-services-kudo module to the latest master.
To run tests:
./tests/run.sh(or./run-tests.shif you don't have a Go environment set up).Example run: