Sensorapp integration tests #148
Conversation
Add option to delay client start after server signals being ready
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @FR4NK-W)
camerapp/imagefetcher/imagefetcher_integration_test.go, line 1 at r1 (raw file):
// Copyright 2020 ETH Zurich
Would it be possible to move this under camerapp/ directly or does this cause trouble? It just seems a bit arbitrary to put this under the "fetcher", as it tests both the server and the client.
camerapp/imagefetcher/imagefetcher_integration_test.go, line 50 at r1 (raw file):
_, file, _, _ := runtime.Caller(0) cwd := path.Dir(file)
With go test, the current working directory is the directory of this test, as far as I understand. I think you don't even need the absolute path, or otherwise you could use os.Getwd.
camerapp/imagefetcher/imagefetcher_integration_test.go, line 69 at r1 (raw file):
integration.RegExp("^Done, exiting. Total duration \\d+\\.\\d+m?s$"), nil, },
This first test case seems to be a simplified version of the second. Remove it? Otherwise, the test case names should be different.
camerapp/imagefetcher/imagefetcher_integration_test.go, line 72 at r1 (raw file):
{ "fetch_image", append([]string{"-s", integration.DstAddrPattern + ":" + serverPort, "-output", "/tmp/download.jpg"}, cmnArgs...),
I'd suggest put this into a TempDir which will be deleted at the end of the testrun -- this avoids any issues with cluttering the tmp directory, overwriting existing files etc.
camerapp/imagefetcher/imagefetcher_integration_test.go, line 86 at r1 (raw file):
return false } res = true
Ineffective assignment, will be overwritten below. Actually, I'd suggest to get rid of the named return value in this function and just return.
camerapp/imagefetcher/imagefetcher_integration_test.go, line 91 at r1 (raw file):
// cmp exits with 0 exit status if the files are identical, and err is nil if the exit status is 0 err = cmd.Run() res = err == nil
No problem for now, but perhaps the integration framework could allow for a simple flexible check function that runs after completion of the client. Then this check here could be expressed a bit more naturally than this.
camerapp/imagefetcher/imagefetcher_integration_test.go, line 109 at r1 (raw file):
IAPairs := integration.IAPairs(hostAddr) IAPairs = IAPairs[:1]
Why only 1? Is it so slow?
camerapp/imagefetcher/logo.jpg, line 0 at r1 (raw file):
Can be put under testdata/ (as this appears to be the special "idiomatic" place in golang).
sensorapp/sensorserver/sensorserver_integration_test.go, line 93 at r1 (raw file):
func wrapperCommand(inputSource string, command string, port string) (wrapperCmd string, err error){ wrapperCmd = integration.AppBinPath(fmt.Sprintf("%s_wrapper.sh", serverBin))
Why not just check-in this shell script? Otherwise, this should go to a temporary directory.
There was a problem hiding this comment.
Reviewable status: 2 of 7 files reviewed, 8 unresolved discussions (waiting on @matzf)
sensorapp/sensorserver/sensorserver_integration_test.go, line 93 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Why not just check-in this shell script? Otherwise, this should go to a temporary directory.
Done.
camerapp/imagefetcher/imagefetcher_integration_test.go, line 1 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Would it be possible to move this under
camerapp/directly or does this cause trouble? It just seems a bit arbitrary to put this under the "fetcher", as it tests both the server and the client.
Done.
camerapp/imagefetcher/imagefetcher_integration_test.go, line 50 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
With
go test, the current working directory is the directory of this test, as far as I understand. I think you don't even need the absolute path, or otherwise you could useos.Getwd.
Done.
camerapp/imagefetcher/imagefetcher_integration_test.go, line 69 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
This first test case seems to be a simplified version of the second. Remove it? Otherwise, the test case names should be different.
Done.
camerapp/imagefetcher/imagefetcher_integration_test.go, line 72 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
I'd suggest put this into a TempDir which will be deleted at the end of the testrun -- this avoids any issues with cluttering the tmp directory, overwriting existing files etc.
Done.
camerapp/imagefetcher/imagefetcher_integration_test.go, line 86 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Ineffective assignment, will be overwritten below. Actually, I'd suggest to get rid of the named return value in this function and just return.
Done.
camerapp/imagefetcher/imagefetcher_integration_test.go, line 109 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Why only 1? Is it so slow?
Done.
camerapp/imagefetcher/logo.jpg, line at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Can be put under
testdata/(as this appears to be the special "idiomatic" place in golang).
Not directly, imageserver only serves ".", which was camerapp/imagefetcher because the integration test file resided there.
Added a flag to imageserver to serve an arbitrary directory.
Done.
There was a problem hiding this comment.
Reviewed 5 of 5 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @FR4NK-W)
sensorapp/sensorserver/sensorserver_integration_test.go, line 93 at r1 (raw file):
Previously, FR4NK-W wrote…
Done.
This is not cleaned up now. I'd suggest to create the TempDir in the top-level function, add defer os.RemoveAll(dir) and pass output directory (or filename) to this function.
sensorapp/sensorserver/sensorserver_integration_test.go, line 1 at r2 (raw file):
// Copyright 2020 ETH Zurich
Analogous to the camerapp, move the test under sensorapp, not the server.
camerapp/camerapp_integration_test.go, line 58 at r2 (raw file):
if err != nil { t.Fatalf("Error during setup err: %v", err) }
The temporary directory is not cleaned on errors. Just defer os.RemoveAll(dir) should do.
camerapp/imagefetcher/logo.jpg, line at r1 (raw file):
Previously, FR4NK-W wrote…
Not directly, imageserver only serves ".", which was
camerapp/imagefetcherbecause the integration test file resided there.
Added a flag to imageserver to serve an arbitrary directory.Done.
Sorry, what I actually meant was testdata/ under where the test is located. That would be camerapp/testdata/. The new -d flag would still be needed, but you wouldn't need the AppTestdataPath then.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved
camerapp/camerapp_integration_test.go, line 58 at r2 (raw file):
Previously, matzf (Matthias Frei) wrote…
The temporary directory is not cleaned on errors. Just
defer os.RemoveAll(dir)should do.
Done.
sensorapp/sensorserver/sensorserver_integration_test.go, line 93 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
This is not cleaned up now. I'd suggest to create the TempDir in the top-level function, add
defer os.RemoveAll(dir)and pass output directory (or filename) to this function.
Done.
sensorapp/sensorserver/sensorserver_integration_test.go, line 1 at r2 (raw file):
Previously, matzf (Matthias Frei) wrote…
Analogous to the camerapp, move the test under
sensorapp, not the server.
Done.
Integration tests for camerapp and sensorapp
Adds a clientDelay parameter to RunTests
This change is