Add HttpFetcher component #328
Conversation
Add class HttpFetcher fetches data from URL endpoint. Add the FakeHttpFetcher. Use HttpFetcher on GIDSignIn for revoking the grant scope.
henryhl22321
added this to the Clean up the branch refactorGSI, refactor and merge the components back into main branch milestone
Remove the outdated import path.
|
Hey @henryhl22321 it looks like you have some linting failiures: You may be able to diagnose these failures more quickly by running |
| /// @param data The NSData returned if succeed, | ||
| /// @param error The error returned if failed. | ||
| typedef void(^GIDHTTPFetcherFakeResponseProviderBlock)(NSData *_Nullable data, | ||
| NSError *_Nullable error); |
| typedef void (^GIDHTTPFetcherTestBlock)(NSURLRequest *request, | ||
| GIDHTTPFetcherFakeResponseProviderBlock responseProvider); | ||
|
|
||
| @interface GIDFakeHTTPFetcher : NSObject <GIDHTTPFetcher> |
There was a problem hiding this comment.
Consider renaming GIDFakeHTTPFetcher to GIDHTTPFetcherFake.
| @interface GIDFakeHTTPFetcher : NSObject <GIDHTTPFetcher> | ||
|
|
||
| /// Set the test block which provides the response value. | ||
| - (void)setTestBlock:(GIDHTTPFetcherTestBlock)block; |
There was a problem hiding this comment.
Can this be passed into the initializer instead?
| withComment:(NSString *)comment | ||
| completion:(void (^)(NSData *_Nullable, NSError *_Nullable))completion { | ||
| NSAssert(self.testBlock != nil, @"Set the test block before invoking this method."); | ||
| self.testBlock(urlRequest, ^(NSData *_Nullable data, NSError *_Nullable error) { |
There was a problem hiding this comment.
Do you do anything with urlRequest?
|
|
||
| NS_ASSUME_NONNULL_BEGIN | ||
|
|
||
| // Maximum retry interval in seconds for the fetcher. |
There was a problem hiding this comment.
Make this a doc comment with ///. That will improve readability within this file: option-clicking on kFetcherMaxRetryInterval will show the text on line 9.
| } | ||
|
|
||
| // Verifies disconnect if access token is present. | ||
| - (void)testDisconnectNoCallback_accessToken { | ||
| - (void)testDisconnectNoCallback_accessTokenIsPresent { |
| } | ||
|
|
||
| // Verifies disconnect calls callback with no errors if refresh token is present. | ||
| - (void)testDisconnect_refreshToken { | ||
| - (void)testDisconnect_refreshTokenIsPresent { |
| [self verifyAndRevokeToken:kAccessToken | ||
| hasCallback:YES | ||
| waitingForExpectations:@[accessTokenExpectation]]; | ||
| [_authorization verify]; |
There was a problem hiding this comment.
Why are you removing the calls to verify? It looks like you're still using mocks in GIDSignInTest, but are removing expectations and verifications that what is expected is called. I think these changes run the risk of making the tests weaker: we are still mocking and and also removing some of the guardrails around the mocks.
Is there a way to no longer mock in these tests (here and throughout)? That is one of the main goals of this refactor.
| XCTAssertNil(jsonError, @"must provide valid data"); | ||
| } | ||
| [_fetcherService.fetchers[0] didFinishWithData:data error:error]; | ||
| - (void)notDoFetch { |
There was a problem hiding this comment.
Could this be renamed doNotFectch?
| [[[_authorization expect] andReturn:_authState] authState]; | ||
| [[[_authState expect] andReturn:_tokenResponse] lastTokenResponse]; | ||
| [[[_tokenResponse expect] andReturn:kAccessToken] accessToken]; | ||
| [[[_authorization expect] andReturn:_fetcherService] fetcherService]; | ||
| OCMStub([_authorization fetcherService]).andReturn(_fetcherService); |
There was a problem hiding this comment.
I don't think you need to stub out the fetcherService on the _authorization. It is declared as a public var on GTMAuthSession. Is there a way that you can leverage that to reduce some of the mocking in these tests?
Add class HttpFetcher fetches data from URL endpoint. Add the FakeHttpFetcher.
Use HttpFetcher on GIDSignIn for revoking the grant scope.