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

Fix Alamofire validation not being performed on uploadMultipart #1591

Merged
merged 7 commits into from Feb 24, 2018

Conversation

Projects
None yet
5 participants
@SD10
Member

SD10 commented Feb 22, 2018

Resolves #1590

Alamofire validation was not being performed on .uploadMultipart requests. I've added a backing unit test for this.

@SD10 SD10 changed the title from Bugfix/multipart validation to Fix Alamofire validation not being performed on uploadMultipart Feb 22, 2018

@SD10 SD10 requested review from BasThomas and sunshinejr Feb 22, 2018

@BasThomas

Looks good to me :)

extension HTTPBin {
static func createMultipartFormData() -> [MultipartFormData] {
let url = Bundle(for: MoyaProviderSpec.self).url(forResource: "testImage", withExtension: "png")!

This comment has been minimized.

@BasThomas

BasThomas Feb 22, 2018

Contributor

I know it's a helper, but maybe fatalError here too?

This comment has been minimized.

@SD10

SD10 Feb 22, 2018

Member

Sounds like a good idea to me 👍

switch error {
case .underlying(_, let response):
let validCodes = target.validationType.statusCodes
expect(validCodes).toNot(contain(response!.statusCode))

This comment has been minimized.

@BasThomas

BasThomas Feb 22, 2018

Contributor

Can we assume to always have a response here?

This comment has been minimized.

@SD10

SD10 Feb 22, 2018

Member

I think our hands are tied on this one. Response validation is only done on Alamofire integration tests. So I can't stub and ensure a response 😕 This could be an improvement

@codecov-io

This comment has been minimized.

codecov-io commented Feb 22, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1591   +/-   ##
=======================================
  Coverage   82.05%   82.05%           
=======================================
  Files           5        5           
  Lines         156      156           
=======================================
  Hits          128      128           
  Misses         28       28

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 77cd637...77f8622. Read the comment docs.

@MoyaBot

This comment has been minimized.

MoyaBot commented Feb 22, 2018

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

Generated by 🚫 Danger

@@ -162,7 +162,9 @@ private extension MoyaProvider {
self.cancelCompletion(completion, target: target)
return
}
cancellable.innerCancellable = self.sendAlamofireRequest(alamoRequest, target: target, callbackQueue: callbackQueue, progress: progress, completion: completion)
let validationCodes = target.validationType.statusCodes
let validated = validationCodes.isEmpty ? alamoRequest : alamoRequest.validate(statusCode: validationCodes)

This comment has been minimized.

@sunshinejr

sunshinejr Feb 22, 2018

Member

validatedRequest?

This comment has been minimized.

@SD10

SD10 Feb 22, 2018

Member

Ugh, that is what I had originally. I knew I should've left it 😆

let formData = HTTPBin.createMultipartFormData()
let target = HTTPBin.validatedUploadMultipart(formData, nil)
let provider = MoyaProvider<HTTPBin>()
provider.request(target) { result in

This comment has been minimized.

@sunshinejr

sunshinejr Feb 22, 2018

Member

I would restructure this testing code a little bit. Instead of using expects inside the closures, we could use waitUntil structure. This makes sure we don't skip this test, where with using expects inside the closure we are risking the situation where this test is skipped, e.g. when closure is not executed. See tests above for examples.

This comment has been minimized.

@SD10

SD10 Feb 22, 2018

Member

Great catch. I totally forgot about that

@sunshinejr

It looks pretty good! Thanks for taking care of it. Just a few minor issues I left in the comments.

@@ -299,6 +299,30 @@ final class MoyaProviderIntegrationTests: QuickSpec {
}
}
}
// Resolves ValidationType not working with multipart upload #1590

This comment has been minimized.

@sunshinejr
var validationType: ValidationType {
switch self {
case .validatedUploadMultipart:

This comment has been minimized.

@sunshinejr

sunshinejr Feb 22, 2018

Member

hmm, how about adding customCodes as an associated value, so we can provide it in tests? we could also comment in tests why do we use this constant

This comment has been minimized.

@SD10

SD10 Feb 22, 2018

Member

I can do that 👌. To be honest the reason why this is such a hack of a test is I can't validate a stubbed request. Or else I wouldn't need another enum case 😕

// Resolves ValidationType not working with multipart upload #1590
describe("a provider performing a multipart upload with Alamofire validation") {
it("only allows status code 287") {

This comment has been minimized.

@sunshinejr

sunshinejr Feb 22, 2018

Member

how about it returns error for status code different than 287 and then we could add another case it returns valid response for status code 287

This comment has been minimized.

@SD10

SD10 Feb 22, 2018

Member

I wasn't able to test for 287 specifically because I have no control over the actual response since this is not a stub. I just tested for ValidationType.successCodes instead

// MARK: - Upload Multipart Helpers
extension HTTPBin {
static func createMultipartFormData() -> [MultipartFormData] {

This comment has been minimized.

@sunshinejr

sunshinejr Feb 22, 2018

Member

nitpick: we could change the name to include test in it, e.g. createTestMultipartFormData()

describe("a provider performing a multipart upload with Alamofire validation") {
let provider = MoyaProvider<HTTPBin>()
let formData = HTTPBin.createTestMultipartFormData()
it("returns an error for status code different than 287") {

This comment has been minimized.

@sunshinejr

sunshinejr Feb 23, 2018

Member

last one, can we add few empty lines to the code? looks really stacked 😅

e.g. before it(""), in it("") we could use a 3 blocks scenario similar to Act-Act-Assert which would add new line before waitUntil and one before expects.

@SD10

This comment has been minimized.

Member

SD10 commented Feb 24, 2018

@sunshinejr No problem, I added some spacing between the tests. LMK if this is 👌

@sunshinejr

Looks awesome, thanks again!

@sunshinejr sunshinejr merged commit 6d57da7 into master Feb 24, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@sunshinejr sunshinejr deleted the bugfix/multipart-validation branch Feb 24, 2018

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