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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Perform validation on stubbed responses #1593

Merged
merged 7 commits into from Feb 25, 2018

Conversation

Projects
None yet
4 participants
@SD10
Copy link
Member

SD10 commented Feb 22, 2018

Resolves #1592

This adds the ability to validate a stubbed response.

Something to consider:

The EndpointSampleResponse has a case .response(HTTPURLResponse, Data).

We have a unit test for this where we use the URLResponse initializer to create the HTTPURLResponse.

You can see the two initializers below:

// HTTPURLResponse initializer
public init?(url: URL, statusCode: Int, httpVersion HTTPVersion: String?, headerFields: [String: String]?)

// URLResponse initializer
public init(url: URL, mimeType MIMEType: String?, expectedContentLength length: Int, textEncodingName name: String?)

This change would require that you use the HTTPURLResponse initializer because that is the only way to set the statusCode property (it is get-only). The drawback, however, is arguments like mimeType are also get-only.

However, both HTTPURLResponse and URLResponse are open, so you could get around this by subclassing to make a mock response.

Looking for thoughts from @Moya/contributors

@@ -409,8 +409,7 @@ final class MoyaProviderSpec: QuickSpec {
}

it("returns identical sample response") {
// TODO: Fix this failing test
let response = HTTPURLResponse(url: URL(string: "http://example.com")!, mimeType: nil, expectedContentLength: 0, textEncodingName: nil)
let response = HTTPURLResponse(url: URL(string: "http://example.com")!, statusCode: 200, httpVersion: nil, headerFields: nil)!

This comment has been minimized.

@SD10

SD10 Feb 22, 2018

Author Member

This is the change I discuss in the body of the issue above

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 22, 2018

Codecov Report

Merging #1593 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1593   +/-   ##
=======================================
  Coverage   87.82%   87.82%           
=======================================
  Files           5        5           
  Lines         156      156           
=======================================
  Hits          137      137           
  Misses         19       19

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d57da7...3095767. Read the comment docs.

@SD10

This comment has been minimized.

Copy link
Member Author

SD10 commented Feb 24, 2018

Doing a little more research on this - there should be no issue here. Worst case scenario a user would have to implement a convenience initializer on HTTPURLResponse.

@sunshinejr
Copy link
Member

sunshinejr left a comment

Few comments in this one, thanks a lot for covering stubs Steven! 💯

Apart from that we would need a Changelog entry (probably a bug fix?) and make sure docs are in sync as well.

)
}
stubbedProvider = MoyaProvider<GitHub>(endpointClosure: endpointClosure, stubClosure: MoyaProvider.immediatelyStub)
stubbedProvider.request(.zen) { result in

This comment has been minimized.

@sunshinejr

sunshinejr Feb 24, 2018

Member

can we change it to use waitUntil as well here? 😉

var stubbedProvider: MoyaProvider<GitHub>!

context("response contains invalid status code") {
it("returns a failure case") {

This comment has been minimized.

@sunshinejr

sunshinejr Feb 24, 2018

Member

how about returns an error?

describe("a provider for stubbed requests with validation") {
var stubbedProvider: MoyaProvider<GitHub>!

context("response contains invalid status code") {

This comment has been minimized.

@sunshinejr

sunshinejr Feb 24, 2018

Member

few empty lines in these would be awesome as well 😄

expect(validStatusCodes).toNot(beEmpty())
expect(validStatusCodes).to(contain(response.statusCode))
case .failure(let error):
guard case let MoyaError.underlying(_, response) = error else {

This comment has been minimized.

@sunshinejr

sunshinejr Feb 24, 2018

Member

here a failure case is probably a failed test, as we expect only success case, right? and similar in the above test with the success case?

@@ -409,10 +409,9 @@ final class MoyaProviderSpec: QuickSpec {
}

it("returns identical sample response") {
let response = HTTPURLResponse(url: URL(string: "http://example.com")!, mimeType: nil, expectedContentLength: 0, textEncodingName: nil)
let response = HTTPURLResponse(url: URL(string: "http://example.com")!, statusCode: 200, httpVersion: nil, headerFields: nil)!

This comment has been minimized.

@sunshinejr

sunshinejr Feb 24, 2018

Member

makes sense 👍

@SD10 SD10 force-pushed the enhancement/stubbed-validation branch from 2cb2ee2 to 65c5f0b Feb 25, 2018

@SD10

This comment has been minimized.

Copy link
Member Author

SD10 commented Feb 25, 2018

Alright @sunshinejr, I cleaned up the unit tests and added a CHANGELOG entry

@MoyaBot

This comment has been minimized.

Copy link

MoyaBot commented Feb 25, 2018

1 Warning
⚠️ Consider adding supporting documentation to this change. Documentation can be found in the docs directory.

Generated by 🚫 Danger

[#1591](https://github.com/Moya/Moya/pull/1591) by [@SD10](https://github.com/SD10).
- Fixed `Response` validation not being performed on stubbed requests.

This comment has been minimized.

@sunshinejr

sunshinejr Feb 25, 2018

Member

can we phrase it the same as the entry above? it is basically the same thing, just fixed for different parts of the library, but might seem like something different

This comment has been minimized.

@SD10

SD10 Feb 25, 2018

Author Member

I didn't mention Alamofire because technically it doesn't have anything to do with Alamofire 😂 They don't need to know that though

@sunshinejr
Copy link
Member

sunshinejr left a comment

Awesome work! Just the wording in Changelog and we can 🚢 it 🎉

@sunshinejr sunshinejr merged commit 68a811f into master Feb 25, 2018

1 check was pending

ci/circleci CircleCI is running your tests
Details

@sunshinejr sunshinejr deleted the enhancement/stubbed-validation branch Feb 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment