Add Tests #22
Add Tests #22
Conversation
It was difficult to test ToopherAPI because ToopherAPI#authenticate instantiated AuthenticationStatus objects. It's now possible to provide an AuthenticationStatusFactory to ToopherAPI. To facilitate optional arguments to ToopherAPI, there's now a ToopherAPI builder that all of the existing constructors dump their arguments into, and a ToopherAPI constructor which takes a ToopherAPI.Builder. There's a mock factory and an example test (that should be renamed or removed) that demonstrates the use of these new classes.
* feature/builders: Add factories and builders all over the places Conflicts: src/test/java/com/toopher/TestToopherAPI.java
| } | ||
|
|
||
| public Builder setTerminalNameExtra(String terminalNameExtra) { | ||
| return addExtra("terminal_name_extra", terminalNameExtra); |
There was a problem hiding this comment.
Why not check for null and not empty for this and the following property?
There was a problem hiding this comment.
This is code from ToopherAPI that I refactored to use builders and factories. There wasn't a null check in the original code and I didn't want to add a null check for fear of changing the behavior, since we didn't have tests around it.
| this.reason = reason; | ||
| this.terminalId = terminalId; | ||
| this.terminalName = terminalName; | ||
| } |
There was a problem hiding this comment.
What would the json parameter contain in this case? Without reviewing the code more thoroughly this method seems like it would simply create an object from json then set the same properties again.
There was a problem hiding this comment.
json is the JSONObject, like AuthenticationStatus#_raw_data (or whatever) in toopher-python. Off the top of my head, the only thing that the call to super does is save json as an instance variable.
| toopherApi.authenticate(pairingId, terminalName); | ||
| } catch(RequestError re){ | ||
| assertTrue(true); | ||
| } |
There was a problem hiding this comment.
It looks like people prefer to put the failure under the method that should throw an exception. See http://stackoverflow.com/a/156868/553403
try {
toopherApi.authenticate(pairingId, terminalName);
fail("My method didn't throw when I expected it to");
} catch(RequestError re){
assertTrue(true);
}
There was a problem hiding this comment.
A test without any assertions passes, so
- without a call to
fail, this test doesn't actually test anything, and assertTrue(true)is unnecessary.
|
I'm glad that toopher-java is getting some much-needed attention! I'd like to see this pull request made a bit more consistent before it's merged. @smholloway pointed out some formatting issues, and hinted that the style of the tests changes from the first test to the last, since you refactored HttpClientMock and I refactored ToopherAPI as you were writing writing them. For example, later tests
while earlier tests don't. Not every test tests the same thing, so some of these might not be applicable in some cases, but many of them should be quite similar to each other. |
| .build(); | ||
|
|
||
| try { | ||
| api.authenticate(PAIRING_ID, TERMINAL_NAME); |
There was a problem hiding this comment.
@smholloway Here's an example of using TERMINAL_NAME.
|
I'm really liking where this is headed - thanks to all the contributors! |
|
Rereading the conversation, I think this is good to merge. The things I pointed out don’t invalidate this pull request and can be implemented as future work, but +30% test coverage! |
Adds tests. Increases test coverage from 30% to 70% 😸 See https://coveralls.io/r/toopher/toopher-java