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

Make Sessions throw meaningful message when used with incompatible Perf #5477

Merged
merged 9 commits into from
Oct 30, 2023

Conversation

mrober
Copy link
Contributor

@mrober mrober commented Oct 26, 2023

No description provided.

}

@Test
fun handleSessionUpdate_sendsToAllSubscribersAsLongAsOneIsEnabled() =
runTest(UnconfinedTestDispatcher()) {
val client = SessionLifecycleClient(backgroundDispatcher() + coroutineContext)
val crashlyticsSubscriber = addSubscriber(true, SessionSubscriber.Name.CRASHLYTICS)
val perfSubscriber = addSubscriber(false, SessionSubscriber.Name.PERFORMANCE)
val mattSaysHiSubscriber = addSubscriber(false, SessionSubscriber.Name.MATT_SAYS_HI)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Matt.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 26, 2023

Coverage Report 1

Affected Products

  • firebase-sessions

    Overall coverage changed from ? (d9bdc42) to 73.81% (05e1a2e) by ?.

    26 individual files with coverage change

    FilenameBase (d9bdc42)Merge (05e1a2e)Diff
    ApplicationInfo.kt?100.00%?
    AutoSessionEventEncoder.java?100.00%?
    Comparisons.kt?100.00%?
    Emitters.kt?0.00%?
    EventGDTLogger.kt?75.00%?
    FirebaseSessions.kt?0.00%?
    FirebaseSessionsDependencies.kt?85.71%?
    FirebaseSessionsRegistrar.kt?0.00%?
    LocalOverrideSettings.kt?100.00%?
    RemoteSettings.kt?88.57%?
    RemoteSettingsFetcher.kt?68.29%?
    SafeCollector.common.kt?0.00%?
    SessionDatastore.kt?3.13%?
    SessionEvent.kt?100.00%?
    SessionEvents.kt?97.87%?
    SessionFirelogPublisher.kt?80.49%?
    SessionGenerator.kt?91.67%?
    SessionLifecycleClient.kt?91.78%?
    SessionLifecycleService.kt?82.67%?
    SessionLifecycleServiceBinder.kt?7.69%?
    SessionsActivityLifecycleCallbacks.kt?55.56%?
    SessionsSettings.kt?98.08%?
    SessionSubscriber.kt?100.00%?
    SettingsCache.kt?94.83%?
    SettingsProvider.kt?50.00%?
    TimeProvider.kt?50.00%?

Test Logs

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

@github-actions
Copy link
Contributor

github-actions bot commented Oct 26, 2023

Unit Test Results

   140 files   -    772     140 suites   - 772   3m 46s ⏱️ - 37m 27s
1 070 tests  - 4 005  1 070 ✔️  - 3 984  0 💤  - 21  0 ±0 
2 140 runs   - 8 095  2 140 ✔️  - 8 053  0 💤  - 42  0 ±0 

Results for commit 86d6033. ± Comparison against base commit d9bdc42.

♻️ This comment has been updated with latest results.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 26, 2023

Size Report 1

Affected Products

  • base

    TypeBase (d9bdc42)Merge (05e1a2e)Diff
    apk (aggressive)?8.39 kB? (?)
    apk (release)?8.66 kB? (?)
  • firebase-sessions

    TypeBase (d9bdc42)Merge (05e1a2e)Diff
    aar?152 kB? (?)
    apk (aggressive)?373 kB? (?)
    apk (release)?2.08 MB? (?)

Test Logs

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

@mrober mrober requested a review from visumickey October 27, 2023 16:36
Comment on lines +45 to +49
Incompatible versions of Firebase Perf and Firebase Sessions.
A safe combination would be:
firebase-sessions:1.1.0
firebase-crashlytics:18.5.0
firebase-perf:20.5.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want the message to reflect that a particular version of a library and above or just leave it at the specified version. Leaving it at a specified version is going to need to update this comment in the near future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put this for now because I don't know what will happen in future versions. I expect we will make it compatible again, so maybe only this version will have this message. If you have a better idea for wording that reflects that we can work on the message.

@mrober mrober merged commit e80997f into sessions-nine Oct 30, 2023
21 of 22 checks passed
@mrober mrober deleted the sessions-nine-matt-says-hi branch October 30, 2023 19:52
@firebase firebase locked and limited conversation to collaborators Nov 30, 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.

4 participants