XcodeServerEndpoints.endpointURL tests

[pmkowal]

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.

[buildasaur]

Result of Integration 1

Duration: 1 minute and 4 seconds
Result: Perfect build! All 56 tests passed. 👍
Test Coverage: 54%.

[czechboy0]

Hmm, that looks weird. I would expect that url to be just /api/bots/botValue/integrations. AFAIK, rev is only used when deleting a bot. Maybe you found a bug 😆

[czechboy0]

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 :)

[pmkowal]

Thanks for comments @czechboy0 😃 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 😉.

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.

[buildasaur]

Result of Integration 2

Duration: 48 seconds
Result: Perfect build! All 56 tests passed. 👍
Test Coverage: 53%.

[pmkowal]

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 😄

[czechboy0]

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 :)

[czechboy0]

Here, for example. Why do we need 3 separate cases?

[pmkowal]

[
    "integration": "integrationValue",
]

This is the desired set of parameters when it comes to /api/integrations/integrationValue path creation.

[
    "otherKey": "otherValue",
    "integration": "integrationValue"
]

In this case test checks if additional/random parameter won't affect path creation. Currently it's kind of obvious, but in the future someone can change the implementation and it's an additional protection.

[
    "rev": "revValue",
    "otherKey": "otherValue",
    "integration": "integrationValue"
]

This case was meant to test if adding rev key without bot key won't add revValue to the path, but it can be removed since testEndpointURLCreationForBots tests it.

[czechboy0]

Okay, if you feel like there's anything that can be removed, can you please do it? In the meantime I'll look at the rev stuff. Thanks!

[pmkowal]

Ok, thanks for review, will do it.

[czechboy0]

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 😊

Feel free to fix it whenever you have time 😉

[pmkowal]

Ok, thanks! After I make fixes to the old tests I will merge #82 and improve tests 😉

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?

[czechboy0]

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.)

[buildasaur]

Result of Integration 3

Duration: 48 seconds
Result: Perfect build! All 44 tests passed. 👍
Test Coverage: 50%.

[buildasaur]

Result of Integration 4

Duration: 45 seconds
Result: Perfect build! All 45 tests passed. 👍
Test Coverage: 51%.

[pmkowal]

@czechboy0 I made couple of fixes:

• leaving tests which test success paths only
• add let expectation to each test to make them more readable
• "/api/bots/bot_id/rev_id" path uses rev only when method is DELETE

Please review.

[czechboy0]

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.

[czechboy0]

Thanks so much, @pmkowal! 👍

[pmkowal]

Cool, thanks @czechboy0 !

[cojoj]

👏

Officially, we've got a new contributor! 🎉

[czechboy0]

🎆

[pmkowal]

😄