Conversation
…sed assertions and added assertNetworkRequestsCount
| /// The error associated with the request failure or nil on success | ||
| public let error: Error? | ||
|
|
||
| public init(data: Data?, response: HTTPURLResponse?, error: Error?) { |
There was a problem hiding this comment.
Had to make this public to create a HttpConnection as by default it is internal, will need to make this change in swift core too.
nporter-adbe
left a comment
There was a problem hiding this comment.
looks good, just a few questions.
| extension NetworkRequest : Hashable { | ||
|
|
||
| /// Equals compare based on host, scheme and URL path. Query params are not taken into consideration | ||
| public static func == (lhs: NetworkRequest, rhs: NetworkRequest) -> Bool { |
There was a problem hiding this comment.
Why are query params not considered? I think if you declare that NetworkRequest conforms to Equatable you will get this functionality for free without having to implement it yourself.
There was a problem hiding this comment.
I implemented a custom compare for simplicity, I don't think you want to pass all the parameters when you mock a response (including headers, read/connect timeout).
Regarding the query params, some of the network requests contain unique query params that cannot be fetched from the extension beforehand, for e.g. requestId.
The other option would be to pass a regex, but again that makes the usage a bit more complex. With current implementation you can assert on these parameters on the NetworkRequest (s) returned by getNetworkRequestsWith(url: url, httpMethod: HttpMethod.post), if needed.
.circleci/config.yml
Outdated
| - run: | ||
| name: Project Setup | ||
| command: make setup | ||
| command: make update |
There was a problem hiding this comment.
I don't agree with calling pod update in CI as it could update the pods to versions the developer did not intend. Instead, we should update the pods locally and push the new Podfile.lock and call pod install in CI to install the CocoaPods versions defined in Podfile.lock. Also, calling pod repo update takes a long time in CircleCI.
There was a problem hiding this comment.
@kevinlind would pod install work for new versions without repo update?
There was a problem hiding this comment.
I think you mean pod repo update? I talked with Steve about this a while ago because the Places Monitor build does do pod repo update && pod install. I was hoping we could avoid running update as it takes so long in CircleCI, but it looks like it is needed. Instead of changing the CircleCI config, can you change the Makefile setup step to cd build/xcode && pod repo update || true && pod install ?
There was a problem hiding this comment.
I thought about that, but we use make setup on our local machines as well and I didn't want to run pod repo update all the times, it indeed takes long time. I can add a ci-pod-update command in the Makefile instead and use it in the config.
There was a problem hiding this comment.
I don't particularly like having CI specific targets in the Makefile. I know we do that for the internal repos, but that was because we were using common build files which needed a consistent set of target names. Instead, what about adding a new target "install-dep" or "pod-install" which just runs cd build/xcode && pod install and change "setup" to cd build/xcode && pod repo update || true && pod install ?
3cf4855 to
0d92fdd
Compare
0d92fdd to
8d4d46e
Compare
Description
Adds APIs for setting network request expectations, mocking a network response and making assertions on the count and network request format in functional tests.
Related Issue
Motivation and Context
How Has This Been Tested?
See FunctionalSampleTest
Screenshots (if appropriate):
Types of changes
Checklist: