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

Add HttpFetcher component #328

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Add HttpFetcher component #328

wants to merge 2 commits into from

Conversation

henryhl22321
Copy link
Contributor

Add class HttpFetcher fetches data from URL endpoint. Add the FakeHttpFetcher.
Use HttpFetcher on GIDSignIn for revoking the grant scope.

Add class HttpFetcher fetches data from URL endpoint.
Add the FakeHttpFetcher.
Use HttpFetcher on GIDSignIn for revoking the grant scope.
Remove the outdated import path.
@mdmathias
Copy link
Collaborator

Hey @henryhl22321 it looks like you have some linting failiures:

- ERROR | xcodebuild:  /Users/runner/work/GoogleSignIn-iOS/GoogleSignIn-iOS/GoogleSignIn/Tests/Unit/GIDHTTPFetcherTest.m:48:3: error: declaration of 'GTMSessionFetcherTestBlock' must be imported from module 'GTMSessionFetcher.GTMSessionFetcher' before it is required

You may be able to diagnose these failures more quickly by running pod lib lint GoogleSignIn.podspec locally before sending up your PR for review.

/// @param data The NSData returned if succeed,
/// @param error The error returned if failed.
typedef void(^GIDHTTPFetcherFakeResponseProviderBlock)(NSData *_Nullable data,
NSError *_Nullable error);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent one space.

typedef void (^GIDHTTPFetcherTestBlock)(NSURLRequest *request,
GIDHTTPFetcherFakeResponseProviderBlock responseProvider);

@interface GIDFakeHTTPFetcher : NSObject <GIDHTTPFetcher>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider renaming GIDFakeHTTPFetcher to GIDHTTPFetcherFake.

@interface GIDFakeHTTPFetcher : NSObject <GIDHTTPFetcher>

/// Set the test block which provides the response value.
- (void)setTestBlock:(GIDHTTPFetcherTestBlock)block;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you do anything with urlRequest?


NS_ASSUME_NONNULL_BEGIN

// Maximum retry interval in seconds for the fetcher.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Camel-case

}

// Verifies disconnect calls callback with no errors if refresh token is present.
- (void)testDisconnect_refreshToken {
- (void)testDisconnect_refreshTokenIsPresent {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Camel-case

[self verifyAndRevokeToken:kAccessToken
hasCallback:YES
waitingForExpectations:@[accessTokenExpectation]];
[_authorization verify];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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.

2 participants