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

Already on GitHub? Sign in to your account

Make URLSession injectable into Networking for easy mocking. #115

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

codeOfRobin
Copy link

Signed-off-by: Robin Malhotra me@rmalhotra.com

Signed-off-by: Robin Malhotra <me@rmalhotra.com>
@codecov-io
Copy link

codecov-io commented Feb 11, 2019

Codecov Report

Merging #115 into master will increase coverage by 0.02%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #115      +/-   ##
==========================================
+ Coverage   92.73%   92.76%   +0.02%     
==========================================
  Files          64       64              
  Lines        3110     3108       -2     
==========================================
- Hits         2884     2883       -1     
+ Misses        226      225       -1
Impacted Files Coverage Δ
Sources/Networking.swift 58.52% <75%> (+0.09%) ⬆️

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 3b9a852...234a5e7. Read the comment docs.

@vadymmarkov
Copy link
Owner

I think it would be better to keep both init methods, one with session configuration and one with session. What do you think?

@codeOfRobin
Copy link
Author

That works too! Have the init with sessionConfiguration generate a URLSession in the init. Would you like me to add that to the PR?

@vadymmarkov
Copy link
Owner

Yes, please 😉

@codeOfRobin
Copy link
Author

One problem now 😕, since both declarations look identical to the compiler,

the empty initializer confuses the compiler 😞

Any way you can think of to get around this? (also, need to fix that vertical_alignment thing that won't go away :/

@vadymmarkov
Copy link
Owner

It should work if you drop default parameter for session: URLSession and create it like Networking(session: .shared) when needed.

Signed-off-by: Robin Malhotra <me@rmalhotra.com>
@codeOfRobin
Copy link
Author

Done 😄

@vadymmarkov
Copy link
Owner

Great! Any idea why tests are failing?

@codeOfRobin
Copy link
Author

@vadymmarkov I just tried running those tests locally. The test that failed on CI (NetworkingSpec.Networking___urlSession_didReceiveChallenge_completionHandler___with_NSURLAuthenticationMethodServerTrust__without_baseUrl__passess_valid_parameters_to_completion() passed over here :/

Could you try running it locally on your computer to see if you can reproduce it?

@codeOfRobin
Copy link
Author

Wait a second, I noticed the tests are running on Xcode 9.4. Could that be a cause?

@codeOfRobin
Copy link
Author

Got a branch with an Xcode 10.1 image in the .travis.yml: https://github.com/codeOfRobin/Malibu/tree/travis-new-image

@codeOfRobin
Copy link
Author

Hi 👋 @vadymmarkov gentle nudge, was hoping you could take a look at this 😬

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