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.

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

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

Merged
merged 9 commits into from
Oct 30, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3bd183c
Make Sessions throw meaningful message when used with incompatible Perf
mrober Oct 26, 2023
ee25e35
Merge branch 'sessions-nine' into sessions-nine-matt-says-hi
mrober Oct 30, 2023
3f46f5b
Format
mrober Oct 30, 2023
670f157
Format x2
mrober Oct 30, 2023
57b1588
Format x3
mrober Oct 30, 2023
0d4e937
Format n+1
mrober Oct 30, 2023
39acdf8
Fix lint warning
mrober Oct 30, 2023
5ed4a64
Fix lint warning n+1
mrober Oct 30, 2023
86d6033
Fix a metric tonne of lint warnings
mrober Oct 30, 2023
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
13 changes: 13 additions & 0 deletions ...sessions/src/main/kotlin/com/google/firebase/sessions/api/FirebaseSessionsDependencies.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,19 @@ object FirebaseSessionsDependencies {
* the Sessions SDK will never generate a session.
*/
fun addDependency(subscriberName: SessionSubscriber.Name) {
if (subscriberName == SessionSubscriber.Name.PERFORMANCE) {
throw IllegalArgumentException(
"""
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
Comment on lines +45 to +49
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.

For more information contact Firebase Support.
"""
.trimIndent()
)
}
if (dependencies.containsKey(subscriberName)) {
Log.d(TAG, "Dependency $subscriberName already added.")
return
Expand Down
19 changes: 9 additions & 10 deletions firebase-sessions/src/test/kotlin/com/google/firebase/sessions/SessionLifecycleClientTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,7 @@ import org.robolectric.Shadows.shadowOf
@MediumTest
@RunWith(RobolectricTestRunner::class)
internal class SessionLifecycleClientTest {

lateinit var fakeService: FakeSessionLifecycleServiceBinder
private lateinit var fakeService: FakeSessionLifecycleServiceBinder

@Before
fun setUp() {
Expand Down Expand Up @@ -189,8 +188,8 @@ internal class SessionLifecycleClientTest {
fun doesNotSendLifecycleEventsWithoutEnabledSubscribers() =
runTest(UnconfinedTestDispatcher()) {
val client = SessionLifecycleClient(backgroundDispatcher() + coroutineContext)
val crashlyticsSubscriber = addSubscriber(false, SessionSubscriber.Name.CRASHLYTICS)
val perfSubscriber = addSubscriber(false, SessionSubscriber.Name.PERFORMANCE)
addSubscriber(collectionEnabled = false, SessionSubscriber.Name.CRASHLYTICS)
addSubscriber(collectionEnabled = false, SessionSubscriber.Name.MATT_SAYS_HI)
client.bindToService()

fakeService.serviceConnected()
Expand All @@ -205,8 +204,8 @@ internal class SessionLifecycleClientTest {
fun sendsLifecycleEventsWhenAtLeastOneEnabledSubscriber() =
runTest(UnconfinedTestDispatcher()) {
val client = SessionLifecycleClient(backgroundDispatcher() + coroutineContext)
val crashlyticsSubscriber = addSubscriber(true, SessionSubscriber.Name.CRASHLYTICS)
val perfSubscriber = addSubscriber(false, SessionSubscriber.Name.PERFORMANCE)
addSubscriber(collectionEnabled = true, SessionSubscriber.Name.CRASHLYTICS)
addSubscriber(collectionEnabled = false, SessionSubscriber.Name.MATT_SAYS_HI)
client.bindToService()

fakeService.serviceConnected()
Expand Down Expand Up @@ -234,31 +233,31 @@ internal class SessionLifecycleClientTest {
runTest(UnconfinedTestDispatcher()) {
val client = SessionLifecycleClient(backgroundDispatcher() + coroutineContext)
val crashlyticsSubscriber = addSubscriber(true, SessionSubscriber.Name.CRASHLYTICS)
val perfSubscriber = addSubscriber(true, SessionSubscriber.Name.PERFORMANCE)
val mattSaysHiSubscriber = addSubscriber(true, SessionSubscriber.Name.MATT_SAYS_HI)
client.bindToService()

fakeService.serviceConnected()
fakeService.broadcastSession("123")

waitForMessages()
assertThat(crashlyticsSubscriber.sessionChangedEvents).containsExactly(SessionDetails("123"))
assertThat(perfSubscriber.sessionChangedEvents).containsExactly(SessionDetails("123"))
assertThat(mattSaysHiSubscriber.sessionChangedEvents).containsExactly(SessionDetails("123"))
}

@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.

client.bindToService()

fakeService.serviceConnected()
fakeService.broadcastSession("123")

waitForMessages()
assertThat(crashlyticsSubscriber.sessionChangedEvents).containsExactly(SessionDetails("123"))
assertThat(perfSubscriber.sessionChangedEvents).containsExactly(SessionDetails("123"))
assertThat(mattSaysHiSubscriber.sessionChangedEvents).containsExactly(SessionDetails("123"))
}

private fun addSubscriber(
Expand Down
8 changes: 4 additions & 4 deletions ...ions/src/test/kotlin/com/google/firebase/sessions/api/FirebaseSessionsDependenciesTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ package com.google.firebase.sessions.api
import androidx.test.ext.junit.runners.AndroidJUnit4
import com.google.common.truth.Truth.assertThat
import com.google.firebase.sessions.api.SessionSubscriber.Name.CRASHLYTICS
import com.google.firebase.sessions.api.SessionSubscriber.Name.PERFORMANCE
import com.google.firebase.sessions.api.SessionSubscriber.Name.MATT_SAYS_HI
import com.google.firebase.sessions.testing.FakeSessionSubscriber
import kotlin.time.Duration.Companion.seconds
import kotlinx.coroutines.Dispatchers
Expand Down Expand Up @@ -71,14 +71,14 @@ class FirebaseSessionsDependenciesTest {

@Test
fun getSubscriber_dependencyAdded_notRegistered_throws() {
FirebaseSessionsDependencies.addDependency(PERFORMANCE)
FirebaseSessionsDependencies.addDependency(MATT_SAYS_HI)

val thrown =
assertThrows(IllegalStateException::class.java) {
FirebaseSessionsDependencies.getSubscriber(PERFORMANCE)
FirebaseSessionsDependencies.getSubscriber(MATT_SAYS_HI)
}

assertThat(thrown).hasMessageThat().contains("Subscriber PERFORMANCE has not been registered")
assertThat(thrown).hasMessageThat().contains("Subscriber MATT_SAYS_HI has not been registered")
}

@Test
Expand Down
2 changes: 1 addition & 1 deletion ...ons/src/test/kotlin/com/google/firebase/sessions/testing/FirebaseSessionsFakeRegistrar.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ internal class FirebaseSessionsFakeRegistrar : ComponentRegistrar {
Component.builder(SessionsSettings::class.java)
.name("sessions-settings")
.add(Dependency.required(firebaseApp))
.add(Dependency.required(firebaseInstallationsApi))
.add(Dependency.required(blockingDispatcher))
.add(Dependency.required(backgroundDispatcher))
.factory { container ->
SessionsSettings(
Expand Down
Loading