Add test for fetching repositories #63
Conversation
| XCTAssertEqual(repos.count, 2, "There should be two repositories available") | ||
|
|
||
| for (index, repo) in repos.enumerate() { | ||
| XCTAssertEqual(repo.name, "Test\(index + 1)") |
There was a problem hiding this comment.
Should we be making the assumption that the order of returned repositories will always be the same? I'm not sure how much is guaranteed in the API. Maybe instead just create a set of names and make sure all are present instead of asserting that they will be there in this exact order.
There was a problem hiding this comment.
Yeah, I've foolishly assumed that because we've recorded a casette we'll always get the same... In that case the other two assertions:
XCTAssertEqual(repos[0].sshAccess, Repository.SSHAccessType.LoggedInReadWrite)
XCTAssertEqual(repos[1].writeAccessExternalIds, [ "ABCDEFAB-CDEF-ABCD-EFAB-CDEF00000050", "D024C308-CEBE-4E72-BE40-E1E4115F38F9" ])also should be more generic?
Let's clarify something... If I've recored a casette with 2 Repositories like now we've no guarantee that another recording (if that ever happen) will be exactly the same... I guess we don't. From my perspective DVR's main purpose is to automate the process of creating HTTP stubs - not coping with regular replacing them with new ones. Other way, I don't think we'll be able to write such generic test cases that will handle everything we put there.
There was a problem hiding this comment.
No, sorry, I didn't explain my point properly. The test is great and this is exactly how we should write them.
My view is that we should test the things that the Xcode Server API is promising to always be true. You have two repos right now, so the API should always return two repos. You have two repos with specific names and properties and again, that's exactly what the API should return.
BUT... the order in which it will return these is not promised by the API in any way. So in theory, if you ran this again on a live server, the order in which the API returns the repos might get reversed. Which is completely legal from the API's perspective, but our test would suddenly fail, because Test2 would come before Test1. Which it shouldn't, because the API is still doing the things it promised to do.
So try keep the tests as specific (not generic) as possible to the things that will always be true in this state of your Xcode Server. But do not test things that are not guaranteed by the API, like the order in which the repositories are returned.
Another example might be integrations and bots. When you ask for integrations, they are guaranteed to be returned in chronological order. So you might assert that the time/number of each integration, when iterating over them, is always going up or down. Bots, on the other hand, don't have a promised ordering, so you shouldn't test that bot A comes before bot B, just because it happened once.
Does this make sense? Tell me if not and I'll try to reword. This is very important that we're on the same page.
So the only change I'm suggesting is that you change the asserts for the correct names from being order-dependent (Test1 coming before Test2) to being order-independent, such as
let repoNames = Set(repos.map { $0.name })
for (index, repo) in repos.enumerate() {
XCTAssertTrue(repoNames.contains("Test\(index + 1)"))
}
This way it doesn't matter the order in which the repos are returned, the test will only make sure that there is a Test1 and Test2 repos returned.
There was a problem hiding this comment.
And when it comes to the role of DVR, this is what I think:
Ideally, when we're writing our tests, we would always have a live Xcode Server accessible that would be setup a certain way and never change its state. So we'd write tests about what we know is true about this live Xcode Server and our tests would always call out and assert all the things we want to be true in its responses.
However, having a live server that never changes available at all times is pretty impractical for many reasons. This is where I see the value of DVR. It lets us snapshot a request + response with our live Xcode Server and then it acts in place of our Xcode Server. Which we can take down and not have it sitting there in a constant state. But our tests should still work even if we ever delete our cassettes and put the original Xcode Server up.
You are correct, DVR also makes it very easy to create the stubs, which is mainly why I really wanted to use it. But I still see it as a tool that lets us take our live servers down and still run our tests unchanged.
There was a problem hiding this comment.
Hahaha, I get your point correctly 😄 I was just overthinking and looking far far ahead and drawing scenarios for future possibilities.
To sum up, I write test, I create it based on my Xcode Server configuration and write tests for this - I don't give a shit what's on your Xcode Server, right?
DVR is cool but I'd be better if all of use use the same instance/configuration of Xcode Server so if any json casette get lost/removed we can easily revert the state back without revriting tests or modifying something on OS X Server - you get my point?
There was a problem hiding this comment.
Exactly, each of us can have a totally different Xcode Server setup and that's ok. The snapshot will depend on who created it, but since the Cassette ends up in git, it'll be very hard to lose it. I get your point, but I wouldn't worry about that too much. If we ever need to regenerate some response again for some reason we'll change it for that one run and then go back to the original config.
That's why it's nice that all Cassettes are independent.
There was a problem hiding this comment.
Okey boss, now we're on the same page 😉
|
Awesome discussion here, @cojoj. I'm glad we talked through the strategy for writing the API tests (which I've never done before, so I'm learning as I go as well). Let me know when you push the changes and I'll merge this beauty. |
|
Same here, first time doing things like this but it's really cool (At work there's always not much time to write stuff like this)! TBH, I won't be happy with this one until Apple fixes this code coverage 😡 |
|
Yeah let's hope they fix it soon. Because I also want to add reporting of code coverage to Buildasaur, so it better be accurate. |
|
Result of integration 1 |
|
Of course they've passed... Some really good folks wrote them 😎 |
|
Testing my new Buildasaur beta with Xcode 7 support. Seems to work so from now on we'll have PR testing! :) |
|
Btw can you make those changes suggested in comments above please, @cojoj? |
|
Give me a sec... Just got to the office and need some coffee ☕️ |
|
Haha sorry didn't mean to rush you, just wanted us to not forget about it :) |
|
@czechboy0 please review... I've made other |
|
Result of integration 2 |
|
Awesome as always, thanks @cojoj! 🎉 |
Add test for fetching repositories
|
And FWIW, our test coverage is 26% right now. But obviously, tests like these will not affect the code coverage metric, but they're extremely important in making sure all the pieces actually fit together (and don't break with future changes). Integration/API tests FTW! |
|
Great! As we're settled and my allergies don't let me lead a normal life I'll probably be bale to write some of these during weekend. Cheers 🍻 |
As I pointed in #45 I've witnessed some twisted behaviors of test coverage - indicates that not all of my code is covered while breakpoints execute in not covered part of code... Maybe some ideas?
Anyway, this code works fine and all tests are passing on both iOS and OS X.