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

Update fetch to utilize latest custom signals #6582

Merged
merged 5 commits into from
Dec 10, 2024

Conversation

tusharkhandelwal8
Copy link
Contributor

@tusharkhandelwal8 tusharkhandelwal8 commented Dec 9, 2024

When we update custom signals using setCustomSignals, the latest custom signals are not retrieved in the subsequent fetch. Currently, we are required to reload the app to fetch them.

Link to the bug: Fetch uses stale custom signal values

Copy link
Contributor

github-actions bot commented Dec 9, 2024

Vertex AI Mock Responses Check ⚠️

A newer major version of the mock responses for Vertex AI unit tests is available. update_responses.sh should be updated to clone the latest version of the responses: v5.2

@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

@tusharkhandelwal8 tusharkhandelwal8 marked this pull request as ready for review December 9, 2024 20:02
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Dec 9, 2024

Coverage Report 1

Affected Products

  • firebase-config

    Overall coverage changed from ? (17cc491) to 83.94% (49efb28) by ?.

    34 individual files with coverage change

    FilenameBase (17cc491)Merge (49efb28)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.94%?
    ConfigFetchHttpClient.java?86.27%?
    ConfigGetParameterHandler.java?96.45%?
    ConfigRealtimeHandler.java?41.38%?
    ConfigRealtimeHttpClient.java?68.93%?
    ConfigSharedPrefsClient.java?87.42%?
    ConfigStorageClient.java?100.00%?
    ConfigUpdate.java?100.00%?
    ConfigUpdateListener.java?0.00%?
    ConfigUpdateListenerRegistration.java?0.00%?
    CustomSignals.java?100.00%?
    DefaultsXmlParser.java?0.00%?
    FirebaseRemoteConfig.java?89.76%?
    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/hgTfqNM2Rl.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Dec 9, 2024

Size Report 1

Affected Products

  • base

    TypeBase (17cc491)Merge (49efb28)Diff
    apk (aggressive)?8.80 kB? (?)
    apk (release)?9.77 kB? (?)
  • firebase-config

    TypeBase (17cc491)Merge (49efb28)Diff
    aar?112 kB? (?)
    apk (aggressive)?211 kB? (?)
    apk (release)?4.59 MB? (?)

Test Logs

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

Copy link
Contributor

github-actions bot commented Dec 9, 2024

Test Results

 42 files  ±0   42 suites  ±0   1m 28s ⏱️ +10s
327 tests +1  327 ✅ +1  0 💤 ±0  0 ❌ ±0 
666 runs  +2  666 ✅ +2  0 💤 ±0  0 ❌ ±0 

Results for commit 4e44147. ± Comparison against base commit 17cc491.

♻️ This comment has been updated with latest results.

@karenyz karenyz self-assigned this Dec 9, 2024
@karenyz
Copy link

karenyz commented Dec 9, 2024

can you add tests for the behavior you are fixing?

@tusharkhandelwal8 tusharkhandelwal8 changed the title Ensure Fetch Uses Latest Custom Signals Update fetch to utilize latest custom signals Dec 10, 2024
Copy link

@ashish-kothari ashish-kothari left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, LGTM!

@@ -766,6 +768,19 @@ public void getInfo_hitsThrottleLimit_throttledStatus() throws Exception {
.isEqualTo(firstFetchedContainer.getFetchTime());
}

@Test
public void customSignals_updated_onSubsequentFetch() throws Exception {
Copy link

Choose a reason for hiding this comment

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

nit: fetch_usesLatestCustomSignals?

fetchCallToHttpClientUpdatesClockAndReturnsConfig(firstFetchedContainer);
fetchHandler.fetch();

verifyCustomSignals(customSignals);
Copy link

Choose a reason for hiding this comment

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

+1 to Ashish's comment, I would also expect to see the actual assertion in this test

@tusharkhandelwal8 tusharkhandelwal8 merged commit 9fa2ac9 into remoteConfigCustomTargeting Dec 10, 2024
34 checks passed
@tusharkhandelwal8 tusharkhandelwal8 deleted the fetch-update branch December 10, 2024 18:58
@firebase firebase locked and limited conversation to collaborators Jan 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants