Added travis and tests #10
Conversation
api_test.go
Outdated
| @@ -0,0 +1,300 @@ | |||
| // go-api - Client for the Cacophony API server. | |||
| // tests against cacophony-api require apiURL to be pointing | |||
| // to a valid cacophony-api server and test-seed.sql to be run | |||
There was a problem hiding this comment.
the above 2 lines probably belong further down the file, not in the license header
api.go
Outdated
| func (r *tokenResponse) message() string { | ||
| if len(r.Messages) > 0 { | ||
| return r.Messages[0] | ||
| } | ||
| return "unknown" | ||
| if r.Success { |
There was a problem hiding this comment.
We're hoping to remove the "success" field sometime soon and just use the HTTP status code. Please update the client code accordingly.
| api.justRegistered = true | ||
| } | ||
| return api | ||
| } |
There was a problem hiding this comment.
thanks very much for getting these tests in place. this is so much better.
db-test-seed.sql
Outdated
| -- sudo -i -u postgres psql cacophonytest -f/db-seed.sql | ||
|
|
||
| --admin user admin//password | ||
| --INSERT INTO "Users" (username, email, password, "globalPermission", "createdAt", "updatedAt") VALUES ('admin_test', '[email protected]', '$2a$10$S..GUBx1zVb6r1QklWu5kOgx5czlAPJM1JxeJ0uHAO3nRRhDtNazm', 'write', now(), now()); |
wait-for-api
Outdated
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| sys.exit(main()) |
There was a problem hiding this comment.
why have you copied this into the go-api project? can't you run it from inside the cloned cacophony-api repo?
config.go
Outdated
| } | ||
| privConfigFilename := privConfigFilename(configFile) | ||
| password, err := ReadPassword(privConfigFilename) | ||
| fileLock := flock.New(filename) |
There was a problem hiding this comment.
I'm pretty sure you don't want to use the config file itself as the lock file. Typically you use a file alongside the file you want to change as a lock file. The way it stands now, the WriteFile call below will overwrite the file, creating a new inode, which will make it look like the file is unlocked at that point.
I suggest you make use a lock file either in /var/lock or by appending ".lock" to the password filename.
config.go
Outdated
| fileLock := flock.New(filename) | ||
| lockCtx, cancel := context.WithTimeout(context.Background(), 3*time.Second) | ||
| defer cancel() | ||
| locked, err := fileLock.TryLockContext(lockCtx, 678*time.Millisecond) |
There was a problem hiding this comment.
please make the timeout and the retry time consts
| return nil, err | ||
| } | ||
| } | ||
| return api, nil |
There was a problem hiding this comment.
thinking about it some more... you want to take a lock before ReadPassword and hold it until after WritePassword might have run because if there is no password and NewAPIFromConfig is called concurrently then you can end up with 2 clients trying to register with different passwords at the same time.
api_test.go
Outdated
| ) | ||
|
|
||
| // tests against cacophony-api require apiURL to be pointing | ||
| // to a valid cacophony-api server and test-seed.sql to be run | ||
|
|
||
| var apiURL = "http://localhost:1080" | ||
| var tokenSuccess = true |
There was a problem hiding this comment.
I don't think this is used any more
api_test.go
Outdated
| ) | ||
|
|
||
| // tests against cacophony-api require apiURL to be pointing | ||
| // to a valid cacophony-api server and test-seed.sql to be run | ||
|
|
||
| var apiURL = "http://localhost:1080" |
api_test.go
Outdated
| var apiURL = "http://localhost:1080" | ||
| var tokenSuccess = true | ||
| var responseHeader = http.StatusOK | ||
| var rawThermalData = randString(100) | ||
| var testConfig = "/var/tmp/go-api-test-config.yaml" |
There was a problem hiding this comment.
instead of assuming /var/tmp is the right place, a better thing to do is use TempDir or TempFile in io/ioutil. Get createTestConfig to use one of these and return the name of the file.
| } | ||
| err = ioutil.WriteFile(testConfig, d, 0600) | ||
| return err | ||
| } |
There was a problem hiding this comment.
A useful pattern for test helpers like this is for them to take the *testing.T so that the can helper can use require.NoError etc to deal with anything that goes wrong. This means the caller doesn't have to worry about error handling, making the tests for concise and readable
There was a problem hiding this comment.
Another useful pattern here would be to return a function which will clean up the config file that was created. This can be used in a defer by the calling test. removeTestConfig wouldn't be needed then.
api_test.go
Outdated
| err := createTestConfig() | ||
| defer removeTestConfig() | ||
|
|
||
| require.Equal(t, nil, err, "Must be able to make test config "+testConfig) |
There was a problem hiding this comment.
require.NoError(t, err) is more appropriate for this
Added travis file and tests,
also fixed issue #9 #5 #1 #4