Skip to content

ConfigCacheClientTest.java: fix flaky test due to race condition #6306

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

Jump to bottom
Merged
merged 1 commit into from
Sep 25, 2024
Merged

ConfigCacheClientTest.java: fix flaky test due to race condition #6306

merged 1 commit into from
Sep 25, 2024

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Sep 25, 2024 •
edited
Loading

Fix a race condition in ConfigCacheClientTest.java causing the test getBlocking_hasNoCachedValueAndFileReadTimesOut_returnsNull to flakily fail.

The failure looks like this:

value of: getBlocking(...)
expected: null
but was : {"configs_key":{"long_param":"1L","string_param":"string_value"},"fetch_time_key":1000,"abt_experiments_key":[],"personalization_metadata_key":{},"template_version_number_key":0,"rollout_metadata_key":[]}

The reason for the flake is that sometimes the thread scheduler will run the code that calls ConfigStorageClient.read() before getBlocking() returns; therefore, getBlocking() would return a non-null object in that case. The fix was to cause the mocked read() method to block indefinitely, making it impossible for the read not to timeout.

Here is an example where this flaky failure showed up in CI: https://github.com/firebase/firebase-android-sdk/pull/6290/checks?check_run_id=30611216190

#no-changelog

@dconeybe
ConfigCacheClientTest.java: fix race condition causing test to flakil…
40ffdc0 
…y fail: getBlocking_hasNoCachedValueAndFileReadTimesOut_returnsNull
@dconeybe dconeybe self-assigned this Sep 25, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Sep 25, 2024 •
edited
Loading

📝 PRs merging into main branch

Our main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released.

1 similar comment
@github-actions
Copy link
Contributor

📝 PRs merging into main branch

Our main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released.

@google-oss-bot
Copy link
Contributor

1 Warning
⚠️ Did you forget to add a changelog entry? (Add the 'no-changelog' label to the PR to silence this warning.)

Generated by 🚫 Danger

@google-oss-bot
Copy link
Contributor

Coverage Report 1

Affected Products

  • firebase-config

    Overall coverage changed from ? (2f851c7) to 84.11% (8dc5b95) by ?.

    33 individual files with coverage change

    FilenameBase (2f851c7)Merge (8dc5b95)Diff
    AutoValue_ConfigUpdate.java?29.41%?
    Code.java?0.00%?
    ConfigAutoFetch.java?87.74%?
    ConfigCacheClient.java?93.33%?
    ConfigContainer.java?94.29%?
    ConfigFetchHandler.java?92.86%?
    ConfigFetchHttpClient.java?87.33%?
    ConfigGetParameterHandler.java?96.45%?
    ConfigMetadataClient.java?92.31%?
    ConfigRealtimeHandler.java?41.38%?
    ConfigRealtimeHttpClient.java?68.93%?
    ConfigStorageClient.java?100.00%?
    ConfigUpdate.java?100.00%?
    ConfigUpdateListener.java?0.00%?
    ConfigUpdateListenerRegistration.java?0.00%?
    DefaultsXmlParser.java?0.00%?
    FirebaseRemoteConfig.java?89.52%?
    FirebaseRemoteConfigClientException.java?75.00%?
    FirebaseRemoteConfigException.java?95.65%?
    FirebaseRemoteConfigFetchThrottledException.java?100.00%?
    FirebaseRemoteConfigInfo.java?0.00%?
    FirebaseRemoteConfigInfoImpl.java?100.00%?
    FirebaseRemoteConfigServerException.java?68.42%?
    FirebaseRemoteConfigSettings.java?61.54%?
    FirebaseRemoteConfigValue.java?0.00%?
    FirebaseRemoteConfigValueImpl.java?84.62%?
    Personalization.java?91.43%?
    RemoteConfig.kt?31.58%?
    RemoteConfigComponent.java?90.70%?
    RemoteConfigConstants.java?0.00%?
    RemoteConfigRegistrar.java?100.00%?
    RolloutsStateFactory.java?95.24%?
    RolloutsStateSubscriptionsHandler.java?100.00%?

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/RmXuYPDxgU.html

@github-actions
Copy link
Contributor

Unit Test Results

  40 files  +  24    40 suites  +24   1m 23s ⏱️ + 1m 8s
309 tests +202  309 ✔️ +202  0 💤 ±0  0 ❌ ±0 
630 runs  +416  630 ✔️ +416  0 💤 ±0  0 ❌ ±0 

Results for commit 40ffdc0. ± Comparison against base commit 2f851c7.

This pull request removes 107 and adds 309 tests. Note that renamed tests count towards both. 
com.google.firebase.vertexai.SchemaTests ‑ basic schema declaration
com.google.firebase.vertexai.SchemaTests ‑ full schema declaration
com.google.firebase.vertexai.StreamingSnapshotTests ‑ citation parsed correctly
com.google.firebase.vertexai.StreamingSnapshotTests ‑ empty content
com.google.firebase.vertexai.StreamingSnapshotTests ‑ http errors
com.google.firebase.vertexai.StreamingSnapshotTests ‑ image rejected
com.google.firebase.vertexai.StreamingSnapshotTests ‑ invalid api key
com.google.firebase.vertexai.StreamingSnapshotTests ‑ invalid json
com.google.firebase.vertexai.StreamingSnapshotTests ‑ long reply
com.google.firebase.vertexai.StreamingSnapshotTests ‑ malformed content
…

com.google.firebase.remoteconfig.ConfigTests ‑ Firebase#remoteConfig should delegate to FirebaseRemoteConfig#getInstance()
com.google.firebase.remoteconfig.ConfigTests ‑ Firebase#remoteConfig should delegate to FirebaseRemoteConfig#getInstance(FirebaseApp, region)
com.google.firebase.remoteconfig.ConfigTests ‑ FirebaseRemoteConfigSettings builder works
com.google.firebase.remoteconfig.ConfigTests ‑ Overloaded get() operator returns default value when key doesn't exist
com.google.firebase.remoteconfig.ConfigTests ‑ Overloaded get() operator returns value when key exists
com.google.firebase.remoteconfig.FirebaseRemoteConfigSettingsTest ‑ toBuilder_withFieldsSet_buildsObjectWithFieldsSet
com.google.firebase.remoteconfig.FirebaseRemoteConfigTest ‑ activate2p_hasAbtExperiments_doesNotCallAbt
com.google.firebase.remoteconfig.FirebaseRemoteConfigTest ‑ activate2p_hasNoAbtExperiments_doesNotCallAbt
com.google.firebase.remoteconfig.FirebaseRemoteConfigTest ‑ activate_callToAbtFails_activateStillSucceeds
com.google.firebase.remoteconfig.FirebaseRemoteConfigTest ‑ activate_configWithRolloutMetadata_storedInActivatedCacheSuccessfully
…

@google-oss-bot
Copy link
Contributor

Size Report 1

Affected Products

No changes between base commit (2f851c7) and merge commit (8dc5b95).

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/qeRGAMx5y3.html

@google-oss-bot
Copy link
Contributor

Startup Time Report 1

Note: Layout is sometimes suboptimal due to limited formatting support on GitHub. Please check this report on GCS.

Notes

Startup Times

  • fire-rc

    DeviceStatisticsDistributions
    oriole-32
    Percentile2f851c78dc5b95DiffSignificant (?)
    p10209 ±292 μs91.5 ±55 μs-117 μs (-56.1%)NO
    p25217 ±300 μs97.7 ±58 μs-119 μs (-55.0%)NO
    p50233 ±311 μs115 ±66 μs-118 μs (-50.5%)NO
    p75262 ±322 μs150 ±99 μs-111 μs (-42.5%)NO
    p90309 ±351 μs194 ±118 μs-114 μs (-37.1%)NO

    20 test runs in comparison
    CommitTest Runs
    2f851c7
    • 2024-09-24_18:43:37.595837_TvXb
    • 2024-09-24_18:43:37.595873_wXmt
    • 2024-09-24_18:43:37.595884_KHwA
    • 2024-09-24_18:43:37.595893_YIPW
    • 2024-09-24_18:43:37.595901_AKek
    • 2024-09-24_18:43:37.595908_BsVZ
    • 2024-09-24_18:43:37.595915_QtiZ
    • 2024-09-24_18:43:37.595922_Etbd
    • 2024-09-24_18:43:37.595929_iNEi
    • 2024-09-24_18:43:37.595935_CTJu
    8dc5b95
    • 2024-09-25_02:49:11.172784_LmhU
    • 2024-09-25_02:49:11.172823_SUBx
    • 2024-09-25_02:49:11.172834_ejpb
    • 2024-09-25_02:49:11.172843_cQgQ
    • 2024-09-25_02:49:11.172851_lXai
    • 2024-09-25_02:49:11.172859_ueiB
    • 2024-09-25_02:49:11.172866_ygGc
    • 2024-09-25_02:49:11.172879_IcWP
    • 2024-09-25_02:49:11.172886_wfXN
    • 2024-09-25_02:49:11.172893_aurM
    redfin-30
    Percentile2f851c78dc5b95DiffSignificant (?)
    p10374 ±514 μs614 ±677 μs+239 μs (+63.9%)NO
    p25420 ±600 μs707 ±794 μs+287 μs (+68.4%)NO
    p50498 ±704 μs850 ±953 μs+351 μs (+70.5%)NO
    p75608 ±834 μs1.09 ±1 ms+481 μs (+79.1%)NO
    p90728 ±950 μs1.34 ±1 ms+615 μs (+84.5%)NO

    20 test runs in comparison
    CommitTest Runs
    2f851c7
    • 2024-09-24_18:43:37.595837_TvXb
    • 2024-09-24_18:43:37.595873_wXmt
    • 2024-09-24_18:43:37.595884_KHwA
    • 2024-09-24_18:43:37.595893_YIPW
    • 2024-09-24_18:43:37.595901_AKek
    • 2024-09-24_18:43:37.595908_BsVZ
    • 2024-09-24_18:43:37.595915_QtiZ
    • 2024-09-24_18:43:37.595922_Etbd
    • 2024-09-24_18:43:37.595929_iNEi
    • 2024-09-24_18:43:37.595935_CTJu
    8dc5b95
    • 2024-09-25_02:49:11.172784_LmhU
    • 2024-09-25_02:49:11.172823_SUBx
    • 2024-09-25_02:49:11.172834_ejpb
    • 2024-09-25_02:49:11.172843_cQgQ
    • 2024-09-25_02:49:11.172851_lXai
    • 2024-09-25_02:49:11.172859_ueiB
    • 2024-09-25_02:49:11.172866_ygGc
    • 2024-09-25_02:49:11.172879_IcWP
    • 2024-09-25_02:49:11.172886_wfXN
    • 2024-09-25_02:49:11.172893_aurM

  • timeToInitialDisplay

    DeviceStatisticsDistributions
    oriole-32
    Percentile2f851c78dc5b95DiffSignificant (?)
    p10202 ±4 ms206 ±3 ms+4.25 ms (+2.1%)NO
    p25208 ±4 ms211 ±3 ms+3.17 ms (+1.5%)NO
    p50215 ±4 ms218 ±4 ms+2.42 ms (+1.1%)NO
    p75223 ±4 ms226 ±3 ms+2.70 ms (+1.2%)NO
    p90231 ±4 ms239 ±7 ms+7.93 ms (+3.4%)NO

    20 test runs in comparison
    CommitTest Runs
    2f851c7
    • 2024-09-24_18:43:37.595837_TvXb
    • 2024-09-24_18:43:37.595873_wXmt
    • 2024-09-24_18:43:37.595884_KHwA
    • 2024-09-24_18:43:37.595893_YIPW
    • 2024-09-24_18:43:37.595901_AKek
    • 2024-09-24_18:43:37.595908_BsVZ
    • 2024-09-24_18:43:37.595915_QtiZ
    • 2024-09-24_18:43:37.595922_Etbd
    • 2024-09-24_18:43:37.595929_iNEi
    • 2024-09-24_18:43:37.595935_CTJu
    8dc5b95
    • 2024-09-25_02:49:11.172784_LmhU
    • 2024-09-25_02:49:11.172823_SUBx
    • 2024-09-25_02:49:11.172834_ejpb
    • 2024-09-25_02:49:11.172843_cQgQ
    • 2024-09-25_02:49:11.172851_lXai
    • 2024-09-25_02:49:11.172859_ueiB
    • 2024-09-25_02:49:11.172866_ygGc
    • 2024-09-25_02:49:11.172879_IcWP
    • 2024-09-25_02:49:11.172886_wfXN
    • 2024-09-25_02:49:11.172893_aurM
    redfin-30
    Percentile2f851c78dc5b95DiffSignificant (?)
    p10250 ±32 ms263 ±3 ms+12.4 ms (+5.0%)NO
    p25256 ±33 ms268 ±3 ms+12.2 ms (+4.8%)NO
    p50263 ±34 ms276 ±3 ms+12.8 ms (+4.9%)NO
    p75271 ±37 ms284 ±4 ms+12.5 ms (+4.6%)NO
    p90280 ±41 ms295 ±6 ms+14.9 ms (+5.3%)NO

    20 test runs in comparison
    CommitTest Runs
    2f851c7
    • 2024-09-24_18:43:37.595837_TvXb
    • 2024-09-24_18:43:37.595873_wXmt
    • 2024-09-24_18:43:37.595884_KHwA
    • 2024-09-24_18:43:37.595893_YIPW
    • 2024-09-24_18:43:37.595901_AKek
    • 2024-09-24_18:43:37.595908_BsVZ
    • 2024-09-24_18:43:37.595915_QtiZ
    • 2024-09-24_18:43:37.595922_Etbd
    • 2024-09-24_18:43:37.595929_iNEi
    • 2024-09-24_18:43:37.595935_CTJu
    8dc5b95
    • 2024-09-25_02:49:11.172784_LmhU
    • 2024-09-25_02:49:11.172823_SUBx
    • 2024-09-25_02:49:11.172834_ejpb
    • 2024-09-25_02:49:11.172843_cQgQ
    • 2024-09-25_02:49:11.172851_lXai
    • 2024-09-25_02:49:11.172859_ueiB
    • 2024-09-25_02:49:11.172866_ygGc
    • 2024-09-25_02:49:11.172879_IcWP
    • 2024-09-25_02:49:11.172886_wfXN
    • 2024-09-25_02:49:11.172893_aurM

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/8A9pnDfsMI/index.html

@dconeybe dconeybe enabled auto-merge (squash) September 25, 2024 04:52
@dconeybe dconeybe merged commit 552132b into main Sep 25, 2024
43 checks passed
@dconeybe dconeybe deleted the dconeybe/ConfigCacheClientTestFlakyTestFix branch September 25, 2024 16:59
@firebase firebase locked and limited conversation to collaborators Oct 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@rlazo rlazo rlazo approved these changes

Assignees

@dconeybe dconeybe

Labels

api: remoteconfig main-merge-ack size/S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dconeybe @google-oss-bot @rlazo