Closed pmkowal closed 9 years ago
Duration: 1 minute and 4 seconds Result: Perfect build! All 56 tests passed. :+1: Test Coverage: 54%.
This is some great stuff, @pmkowal, thanks so much! I added some comments, I'll have to look into the rev
stuff. Also, why do you build up multiple params and test them all for the same thing, like in here: https://github.com/czechboy0/XcodeServerSDK/pull/80/files#diff-e671f822957fa3443397d33de2b2235eR105 ? Just curious :)
Thanks for comments @czechboy0 :smiley: The tests were modeled on the endpointURL
method only. I know that there is a folder called routes
in Xcode
app package so I will also take a look at it :wink:.
Also, why do you build up multiple params and test them all for the same thing, like in here: https://github.com/czechboy0/XcodeServerSDK/pull/80/files#diff-e671f822957fa3443397d33de2b2235eR105 ? Just curious :)
I wanted to check all possibilites if additional/random parameters won't affect the path creation.
Btw. In the meantime I will move a common sets of params to setUp() method.
Duration: 48 seconds Result: Perfect build! All 56 tests passed. :+1: Test Coverage: 53%.
Btw. Weird thing about test coverage - I moved the common params from each method to setUp() method and test coverage changed from 54% to 53% - it looks like test coverage depends on the amount of code :smile:
Hmm yeah, I guess it's a percentage of tested code / all code? Including tests probably. Xcode 7 Code Coverage still needs some love I think :)
Ok, I made the code more explicit. See #82. I made the endpoints ONLY put in the rev
when deleting a bot. This way you can remove a bit of the test code, where you're making sure the right values get put in when rev
is in the params. Sorry that the code made it seem like rev
was always needed, I guess I just never caught this case. Which just proves how crucial tests are here. Thanks again and sorry for the added work :blush:
Feel free to fix it whenever you have time :wink:
Ok, thanks! After I make fixes to the old tests I will merge #82 and improve tests :wink:
Btw. I'm currently checking routes
and testEndpointURLCreationForBotsBotRevIntegrations
test - is it possible to have this kind of path: /api/bots/:bot_id/:rev_id/integrations
?
No, that's what I meant by guarding rev
. Now rev
ONLY gets put in if it's a DELETE
request, which looks like DELETE /api/bots/:bot_id/:rev_id
. Technically, it would be put in even if we had another DELETE
request that used the bots endpoint as a prefix, but that's not the case right now so let's not worry about it. So you can remove all the mentions of rev
apart from the one where we delete bot (which I added a test for in #82.)
Duration: 48 seconds Result: Perfect build! All 44 tests passed. :+1: Test Coverage: 50%.
Duration: 45 seconds Result: Perfect build! All 45 tests passed. :+1: Test Coverage: 51%.
@czechboy0 I made couple of fixes:
let expectation
to each test to make them more readablePlease review.
Great, this is much better, thank you! (And just for the future, you don't have to bother with XCTAssertEqual
's text as the third argument. The first two arguments are descriptive and when the test fails, the generated error basically says that they were not equal. Just to save you some time.)
So basically
XCTAssertEqual(url!, expectation, "endpointURL(.SCM_Branches) should return \(expectation)")
and
XCTAssertEqual(url!, expectation)
give you the same information, so you can just use the second one.
Thanks so much, @pmkowal! :+1:
Cool, thanks @czechboy0 !
👏
Officially, we've got a new contributor! 🎉
:fireworks:
:smile:
I've created tests for endpointURL method from XcodeServerEndpoints class. I set access level of XcodeServerEndpoints.endpointURL method as internal as we discussed in #79 issue.
I also renamed EndPoints -> Endpoints in XcodeServerEndPoints name in order to maintain the compatibility between names of files, classes and variables.
Please review.