Skip to content

fix(app_check): Reduce flakiness in TestGetTokenForcingRefresh #1953

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.

Sign up for GitHub

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

Closed
AustinBenoit wants to merge 1 commit into from
Closed

fix(app_check): Reduce flakiness in TestGetTokenForcingRefresh #1953

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions app_check/integration_test/src/integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,14 +506,14 @@ TEST_F(FirebaseAppCheckTest, TestGetTokenForcingRefresh) {
::firebase::app_check::AppCheck::GetInstance(app_);
ASSERT_NE(app_check, nullptr);
firebase::Future<::firebase::app_check::AppCheckToken> future =
app_check->GetAppCheckToken(true);
app_check->GetAppCheckToken(false);
EXPECT_TRUE(WaitForCompletion(future, "GetToken #1"));
::firebase::app_check::AppCheckToken token = *future.result();
EXPECT_NE(token.token, "");
EXPECT_NE(token.expire_time_millis, 0);

// Wait a bit to make sure the expire time would be different
std::this_thread::sleep_for(std::chrono::milliseconds(500));
std::this_thread::sleep_for(std::chrono::milliseconds(1000));
Comment on lines 515 to +516

@gemini-code-assist gemini-code-assist Bot Oct 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The comment here states that we wait to make sure the expire time would be different, but the subsequent assertion on line 522 actually expects the expire times to be equal (EXPECT_EQ). This comment is misleading for this specific step and should be updated to reflect that we are verifying the cached token is still returned.

Suggested change
// Wait a bit to make sure the expire time would be different
std::this_thread::sleep_for(std::chrono::milliseconds(500));
std::this_thread::sleep_for(std::chrono::milliseconds(1000));
// Wait a bit to verify the cached token is still returned
std::this_thread::sleep_for(std::chrono::milliseconds(1000));


// GetToken with force_refresh=false will return the same token.
firebase::Future<::firebase::app_check::AppCheckToken> future2 =
Expand All @@ -522,7 +522,7 @@ TEST_F(FirebaseAppCheckTest, TestGetTokenForcingRefresh) {
EXPECT_EQ(future.result()->expire_time_millis,
future2.result()->expire_time_millis);

std::this_thread::sleep_for(std::chrono::milliseconds(500));
std::this_thread::sleep_for(std::chrono::milliseconds(1000));

@gemini-code-assist gemini-code-assist Bot Oct 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

This sleep is where we actually need to ensure that the clock has advanced so that the newly forced token gets a different expiration timestamp (especially on desktop platforms with 1-second granularity). Adding a clarifying comment here would improve readability.

Suggested change
std::this_thread::sleep_for(std::chrono::milliseconds(1000));
// Wait a bit to make sure the new token's expire time will be different
std::this_thread::sleep_for(std::chrono::milliseconds(1000));


// GetToken with force_refresh=true will return a new token.
firebase::Future<::firebase::app_check::AppCheckToken> future3 =
Expand Down
Loading