Add a small test which runs a basic plugin #903
Conversation
zubron
force-pushed
the
add-more-tests-to-integration-tests
branch
from
b4beef0 to
01acced
Compare
zubron
force-pushed
the
add-more-tests-to-integration-tests
branch
2 times, most recently
from
0620fda to
4922ef5
Compare
Codecov Report
@@ Coverage Diff @@
## master #903 +/- ##
==========================================
- Coverage 47.42% 46.82% -0.61%
==========================================
Files 76 76
Lines 5155 5096 -59
==========================================
- Hits 2445 2386 -59
- Misses 2560 2574 +14
+ Partials 150 136 -14
Continue to review full report at Codecov.
|
zubron
force-pushed
the
add-more-tests-to-integration-tests
branch
2 times, most recently
from
a0f5716 to
84997c2
Compare
scripts/run_integration_tests.sh
Outdated
|
|
||
| # Build and load the test plugin image | ||
| make -C $DIR/test/integration/testImage | ||
| kind load docker-image --name $cluster sonobuoy/testimage:v0.1 |
There was a problem hiding this comment.
nit; I'd specify this at the top of the file too for visibility.
There was a problem hiding this comment.
All looks pretty good but since our first 'real' test is going into the codebase we should strive to make it look like we want future tests to look (at least partially).
I know that a big part of this PR was really just getting it to run and talk to the cluster though so I'm ok with it for now.
The next PR needs to be cleaning up the test itself to move the boilerplate stuff into helpers that can be reused and then the next PR can actually be useful tests.
| @@ -32,6 +34,42 @@ func findSonobuoyCLI() (string, error) { | |||
| return sonobuoyPath, nil | |||
| } | |||
|
|
|||
| func cleanup(t *testing.T, namespace string) { | |||
| var stdout, stderr bytes.Buffer | |||
| deleteCommand := exec.Command(sonobuoy, "delete", "--wait", "-n", namespace) | |||
There was a problem hiding this comment.
If we have various namespaces for different tests (so we wont have collisions) then I'd say we can not do --wait, what do you think?
It would speed tests up and avoid issues where a namespace fails to terminate promptly (which isn't uncommon for k8s)
There was a problem hiding this comment.
You're right, it doesn't need to wait.
| deleteCommand.Stderr = &stderr | ||
|
|
||
| t.Logf("Cleaning up. Running %q\n", deleteCommand.String()) | ||
| if err := deleteCommand.Run(); err != nil { |
There was a problem hiding this comment.
I think as part of expanding this suite we need to make a handful of helper functions that handle this sort of thing:
- run, log output if error & error a test
- run, return output
It feels like you're having to do too much right now each time you want to execute some sonobuoy action.
I'd rather have a test that looks like
func TestFoo(...){
cmd:="run --plugin foo1 --plugin foo2"
expectInResults := []string{"s1","s2"}
output := mustExecuteCommandAndGetResults(t, cmd)
for _,s:=range expectInResults{
if !strings.Contains(s1, output) {...}
}
}
Once we centralize more of the boilerplate of dealing namespaces, buffers, etc into a few of these helpers we can have tests that look almost like unit tests (e.g. table driven tests to just check for strings/files/etc).
There was a problem hiding this comment.
Yes, I definitely wanted to do that. I had started to do that, then got a bit apprehensive because I was basing the design on one test. I can add some of this now though and refactor later.
There was a problem hiding this comment.
Valid concern; we can follow up with a handful of useful tests and the helpers they need next even in the same PR.
You're right not to want to prematurely make 100 hepler funcs which we dont really need.
test/integration/utils.go
Outdated
| } | ||
|
|
||
| func randomNamespace() string { | ||
| charset := "abcdefghijklmnopqrstuvwxyz" |
There was a problem hiding this comment.
We can do random namespaces, but I know it is a frustration sometimes in tests to not know what test created the ns.
Would it be better to just use "sonobuoy-"+t.Name() as a namespace if we needed it?
|
|
||
| t.Logf("Running %q\n", runCommand.String()) | ||
| if err := runCommand.Run(); err != nil { | ||
| t.Errorf("Sonobuoy exited with an error: %v", err) |
There was a problem hiding this comment.
I guess for our first test, just seeing that it ran and exited 0 is OK. Feels a bit empty but it is just a starting place.
There was a problem hiding this comment.
Sorry, I should have followed up on this PR with a comment. Yes, this is a bit of dull and uninteresting test that doesn't tell us anything more than what we currently have in our CI scripts. I think I just got a bit frustrated with trying to get things working and decided that once it was working, I should just try to get that wrapped up and merged. Admittedly, it was fairly straightforward in the end, I was just frustrated with some silly mistakes I made 🙃
zubron
force-pushed
the
add-more-tests-to-integration-tests
branch
from
84997c2 to
dcf69a9
Compare
This change also modifies our Makefile and CI scripts so that the tests can be run on a separate kind cluster. Signed-off-by: Bridget McErlean <[email protected]>
zubron
force-pushed
the
add-more-tests-to-integration-tests
branch
from
dcf69a9 to
61fb21b
Compare
What this PR does / why we need it:
This change adds a simple test and mostly provides scaffolding to be
able to write further tests using the changes introduced in #907.
It adds a new script
run_integration_tests.shwhich creates a separatecluster if necessary and then runs the tests on it.