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

Implement FIRAppCheck as a shim of GACAppCheck #11721

Merged
merged 15 commits into from
Sep 22, 2023

Conversation

andrewheard
Copy link
Contributor

@andrewheard andrewheard commented Aug 23, 2023

#no-changelog

@github-actions
Copy link
Contributor

github-actions bot commented Aug 23, 2023

Apple API Diff Report

Commit: f514dac
Last updated: Fri Sep 22 09:17 PDT 2023
View workflow logs & download artifacts


FirebaseAppCheck

Protocols

FIRAppCheckProvider
[ADDED] -getLimitedUseTokenWithCompletion:
Swift:
+  optional func getLimitedUseToken () async throws -> FIRAppCheckToken
Objective-C:
+  - ( void ) getLimitedUseTokenWithCompletion : ( nonnull void ( ^ )( FIRAppCheckToken * _Nullable , NSError * _Nullable )) handler ;

@@ -304,7 +218,8 @@ - (void)testGetToken_AppCheckProviderError {
}];

// 3. Wait for expectations and validate mocks.
[self waitForExpectations:expectations timeout:0.5];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test and testInteropGetTokenForcingRefresh_Success have been flaky (both locally and on CI). My hypothesis is that this is due to the inverted tokenNotification expectation which must wait the full duration of the timeout (since it's testing that a notification was not posted).

I've split these into two waits since we want to ensure that the tokenForcingRefresh completion handler is invoked and then give a little time to ensure that no notification is posted after. Will see if this addresses the flakiness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't resolve the issue. Replaced tokenUpdateNotificationWithExpectedToken:isInverted: with a new tokenUpdateNotificationNotPosted to handle the isInverted = YES case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you disable xcpretty for just these tests and spit the full xcodebuild logs into GHA? It's short enough that it's probably fine and it may contain a more complete failure reason. Right now it just says test failed but doesn't say why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (and added back now). Thanks for the debugging help!

Copy link
Contributor

@morganchen12 morganchen12 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1,231 +0,0 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Leaving a note here for record keeping: These tests were removed because they enqueue async token fetch tasks that are run when other tests use waitForExpectations, causing test failures in other test targets. They can be reinstated if necessary, but have been replaced with E2E tests.

@andrewheard andrewheard merged commit fba0245 into app-check-core Sep 22, 2023
48 checks passed
@andrewheard andrewheard deleted the ah/app-check-core-shim branch September 22, 2023 17:58
@firebase firebase locked and limited conversation to collaborators Oct 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants