-
Notifications
You must be signed in to change notification settings - Fork 44
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
Implement NSURLProtocol.stopLoading() for delayed requests #96
Conversation
Current coverage is 99.16% (diff: 100%)@@ master #96 diff @@
==========================================
Files 10 10
Lines 470 477 +7
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 466 473 +7
Misses 4 4
Partials 0 0
|
@@ -101,6 +102,7 @@ public final class Router { | |||
} | |||
|
|||
private var routes: [String : Route] = [:] | |||
private var canceledRequests: [NSURLProtocol] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Careful, this would create a retain cycle between Server and Router objects. Not sure if we need a different approach here rather than storing references to Server (NSURLProtocol).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, right. that's bad ... I'll find a solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I solved the retain cycle by remembering the NSURL
, which should be canceled.
fixed in c10b110
Check code cov, merging this into master would decrease our test coverage a bit: please make sure to add unit tests that cover this new scenario, you can check |
# AppCode | ||
.idea | ||
.idea/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for? Some auto generated files from AppCode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these are meta files generated by AppCode (don't want to have them in the repo).
…S) logic better understandable
…sts for 'stopLoading'
Ok, tasks for me
@joanromano how do you run the unit tests locally? |
CMD+U should be enough, not working for you? |
…uter (avoid accessing private vars)
@MP0w codecov requests that I unit test That's totally nonsense ... what shall I do? I would need to access |
@leviathan we are doing BDD so we want to test the behaviors. You don't need to test that method directly but tests the behavior of canceling a request. While testing the cancel request the lines of private method will be implicitly covered since the private methods are used when you cancel a request. You will just have to create a request and cancelling it before it starts and the private methods involved should be then covered . Let me know if you have any question. |
else { | ||
client.URLProtocol(self, didReceiveResponse: NSHTTPURLResponse(URL: requestURL, statusCode: 200, HTTPVersion: "HTTP/1.1", headerFields: nil)!, cacheStoragePolicy: .AllowedInMemoryOnly) | ||
client.URLProtocolDidFinishLoading(self) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a test NSURLProtocol
just for the unit test target in order to intercept any
outgoing calls that are not registered with Routers (some requests to http://www.test.com
we test against). You shouldn't need to touch the RouterTestServer
at all unless I am missing something.
…ter's request loading gets cancelled.
@joanromano & @MP0w any remarks? |
@leviathan I'm going in holidays for 1 week so I won't have time to help but could you guys make sure the tests are failing when the implementation is removed (not cancelling requests)? For the implementation, I would really prefer a simple flag in the Server created by the system which is (IMO) more clean than keeping track of called requests. For example you could spawn 2 equal request at the same time with same URL (no-sense but to highlight the fragility) and the router would not know which one has to be cancelled. I quickly talked with @joanromano and he known my opinion and his own, he will take care of the review since I won't be available. Little side note: next time let's create separate PR for typos (ty very much) so it's easier to review, that's our fault since we didn't create any contributing guideline (will take care when I'm back). Have a good weekend guys! 🎉 :micdrop: |
The overall list of `Router` objects, which is know to all `KakapoServer` instances. | ||
Visibility is on the `KakapoServer` class level, because `NSURLProtocol` just allows us to register | ||
class elements (no instances are possible). | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to add documentation here since it's a private var -> implementation detail on how the Router and Server communicate and Server keeps storage of routers, nothing that the client needs to be aware of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to keep this comment. How shall I mark it that its not visible to the public? /!* ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing special needed, private prop/methods/types are not accessible anyway. However we try to limit comments , only when really needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is not clear/correct IMHO. Not sure if it's even needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh important: routers
should remain private and yes, I will also say that this comment should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed comment in 60417ca
// intentionally left empty | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this ProtocolClientTest
for? I think we shouldn't need to create it.
Basically, the normal way to test it would be:
- With any implementation, add the test that cancels the data task and expects the request to not get started. Thus, this test will fail.
- Add the implementation to stop the requests in KakapoServer
- Check the test again, should pass.
For the test itself: should be enough to get the task from dataTaskWithURL
and cancel it, and expect the request to not being called. Same as you do in "should not start a request when request gets cancelled"
. We could also add other test with more than one request or with one canceled request and another not cancelled request, but that should be it. I don't think we need more tests with a new NSURLProtocolClient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I noticed the current flaw with the cancelledRequests
list on the Router
instance. Requests with identical URLs cannot be distinguished with this design. Soooo.... I will go the way that @MP0w has suggested (the flag on the server). I'll adopt the tests accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need the ProtocolClientTest
to manually simulate the control flow in the KakapoServer
(see: "should send notifications when loading has finished"
).
Because there's no way for me to access the KakapoServer
instances, which are created automatically be the OS. Thus I create one KakapoServer
instance manually to test drive the start
& stop
logic.
I'd be out for the weekend until Monday evening, I will check again everything then. @leviathan thanks for all your efforts on making this feature happen 👍 |
…erver request be marked as cancelled, adopted tests accordingly,
|
||
Note: calls to `stopLoading()` will set this value to `true` | ||
*/ | ||
public private(set) var requestCancelled:Bool = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this to be publicly readable, as stated before: You shouldn't use this class directly but register a
Routerinstead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nevermind, I just saw the access from the Server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joanromano we need contributors and style guidelines... for this time we can just apply the style ourself (e.g. Space missinggn between : and Bool)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Public should be removed, internal getter is fine to access it from the Router
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reduced variable visibility and removed comment in 60417ca
@leviathan thanks for the time invested on this. Overall looks good but after double checking last day I think I might have to discuss some implementation details with @MP0w before merging. I might re think again the scenario of adding all this logic to the I am out two weeks for holidays and so will review this again together when I am back. Sorry for this taking too long and, again, many thanks for your efforts on making this happen. |
|
||
beforeEach { | ||
router = Router.register(baseURL) | ||
router.latency = latency // very high latency to allow us to stop the request before execution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the request is always asynchronous theoretically also 0 latency should work
@joanromano I think is fine , what are your concerns? Other than the few changes we asked I think it's good. |
@MP0w I don't like having to change the signature of I think it's just not a proper separation of concerns, and it shows in the tests when we need to manually create a |
That test is just to check that the unit is working as expected ... it's basically a mocked KakapoServer |
@joanromano I understand your objections. Because, when This is also the reason why I needed to create a Right now, I'm a bit confused how to proceed with this. Looks like the two of you have to sync. |
…on logic, reduced visibility of variables
@leviathan sorry for the confusion, IMO after fixing those last 2 things it's good to merge, the implementation seems correct and it's working. @leviathan thank you for the time you spent on this, we really appreciate it. I hope you are fine with our approach! |
oh weird, these tests: Are succeeding also when commenting |
Merged 🎉 ty very much @leviathan |
General description
Implemented the
NSURLProtocol#stopLoading()
logic inKakapoServer
After the
latency
feature has been added toRouter
, which allows to delay the request callback reporting, it is now possible that a request may be waiting for its callback, butKakapoServer
gets commanded to cancel the particular request.This pull requests makes the full roundtrip and adds the capability to "not complete" the request, if
KakapoServer
command so.Primarily affected components
KakapoServer
&Router
Depending components
Expected possible Pitfalls
Device and OS test coverage
leviathan
Closes #88