Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Device and Platforms tests #65

Merged
merged 9 commits into from Jul 14, 2015
Merged

Conversation

cojoj
Copy link
Contributor

@cojoj cojoj commented Jul 13, 2015

@czechboy0 I hope you're back from holidays, full of energy and ready to work 馃槅

New batch with Device tests.

First of all, I've added missing fields/properties to Device model - please check them and let me know what do you think and if things marked as Enum? in comments should be moved to DeviceSpecyfication file.

One thing that concerns me about cassetes is that we're only testing gold path and skipping any possible error paths. Do you think we should create test cases and test failing paths as well? If so, how would you write those tests? Record another casette, write mocks or stubs? As we leave it as it is now we won't get full coverage of the code...

@cojoj
Copy link
Contributor Author

cojoj commented Jul 13, 2015

Platforms tests

Same thing here with testing different execution paths...

@cojoj cojoj changed the title Device tests Device and Platforms tests Jul 13, 2015
@buildasaur
Copy link
Collaborator

Result of integration 1
Integration took 47 seconds.
Perfect build! All 25 tests passed. 馃憤

self.modelName = json.stringForKey("modelName")
self.deviceECID = json.optionalStringForKey("deviceECID")
self.modelUTI = json.stringForKey("modelUTI")
if let proxyDevice: NSDictionary = json.optionalForKey("activeProxiedDevice") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just use if let proxyDevice = json.optionalDictionaryForKey("activeProxiedDevice") {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake. Earlier I was trying to achieve this other wa around and this is a small artifact...

@czechboy0
Copy link
Member

Let me know when you push the changes so that we can merge this 馃槈

@cojoj
Copy link
Contributor Author

cojoj commented Jul 14, 2015

Ok, changed something, added other things and here's the result... 馃槈

You haven't answered how to handle failing paths in test cases. Got any idea?

@buildasaur
Copy link
Collaborator

Result of integration 2
Integration took 34 seconds.
Perfect build! All 25 tests passed. 馃憤

@czechboy0
Copy link
Member

Yeah the failing paths are interesting. There is not that much code different for each endpoint. Most of the logic is shared. So I'd rather write tests for XcodeServer's sendRequestWithMethod instead of writing very similar failing paths for each API call. Whadaya think?

@czechboy0
Copy link
Member

Cool, thanks as always, @cojoj!

czechboy0 pushed a commit that referenced this pull request Jul 14, 2015
Device and Platforms tests
@czechboy0 czechboy0 merged commit 0fac2bf into buildasaurs:swift-2 Jul 14, 2015
@cojoj
Copy link
Contributor Author

cojoj commented Jul 14, 2015

Exactly, one complete test case for sendRequestWithMethod() can solve all of our problems with specyfic test cases but to write this test we need to create stub so we can imitate the failing response. You know what I mean?

@cojoj cojoj deleted the device-tests branch July 14, 2015 09:55
@czechboy0
Copy link
Member

Yup, feel free to use any of the available ones, but it needs to be clear we're testing sendRequestWithMethod, not e.g. getBots, if you know what I mean.

@cojoj
Copy link
Contributor Author

cojoj commented Jul 14, 2015

I guess I'd be hard to achieve as sendRequestWithMethod() has internal accessory flag...

@czechboy0
Copy link
Member

Should be fine with @testable import XcodeServerSDK in the test file, shouldn't it?

@cojoj
Copy link
Contributor Author

cojoj commented Jul 14, 2015

Probably... I'll try to write something

@czechboy0
Copy link
Member

@cojoj
Copy link
Contributor Author

cojoj commented Jul 14, 2015

Hah, I think that testing sendRequestWithMethod() won't solve everything. I think what needs to be tested is createRequest() and sendRequest() (which leads to dataTaskWithRequest()).

@czechboy0
Copy link
Member

Why? Just call it on an XcodeServer you get from getRecordingXcodeServer, just like with the other tests.

The only thing sendRequestWithMethod does is basically call createRequest and right away sendRequest on it 馃槉 Or am I missing something?

@cojoj
Copy link
Contributor Author

cojoj commented Jul 14, 2015

Right, it doesn't have wide range of responsibilities but you have to fake HTTP responses to check the failable path eg. To test this code (from sendRequestWithMethod()):

guard let data = data else {
    //no body, but a valid response
    completion(response: httpResponse, body: nil, error: nil)
    return
}

You have to return nil body from self.session.dataTaskWithRequest() and to do this you need to provide prepared NSURLRequest which will return proper response...

Correct me I you think that I'm spinning a yarn...

@czechboy0
Copy link
Member

Ok this makes sense. I guess we should test with separate createRequest and sendRequest in this case and with sendRequestWithMethod for different cases, such as with different error returned, server unavailable, wrong returned content types etc.

@cojoj
Copy link
Contributor Author

cojoj commented Jul 14, 2015

Cool!

Anyway, I hate the idea that all in all we'll have to create manually some fake data 馃槗

@czechboy0
Copy link
Member

馃樋

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants