-
Notifications
You must be signed in to change notification settings - Fork 128
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
Ignore dynamically stored labels if Glean is not initialized #374
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r+wc :) Thanks for fixing this!
...re/android/src/test/java/mozilla/telemetry/glean/private/AccumulationsBeforeGleanInitTest.kt
Outdated
Show resolved
Hide resolved
We have 3 scenarios to consider: * Static labels. No database access needed. We just look at what is in memory. * Dynamic labels, initialized Glean. We look up in the database all previously stored labels in order to keep a maximum of allowed labels. * Dynamic labels, uninitialized Glean. We ignore potentially previously stored labels and just take what the user passed in. This potentially allows creating more than `MAX_LABELS` labels, but only before Glean is initialized.
29897d7
to
2ab9c47
Compare
...re/android/src/test/java/mozilla/telemetry/glean/private/AccumulationsBeforeGleanInitTest.kt
Show resolved
Hide resolved
@@ -33,6 +34,8 @@ class AccumulationsBeforeGleanInitTest { | |||
@Before | |||
fun cleanup() { | |||
Glean.testDestroyGleanHandle() | |||
@Suppress("EXPERIMENTAL_API_USAGE") | |||
Dispatchers.API.setTaskQueueing(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may also want to clear the queue to be extra sure...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but blindly clearing breaks other tests. We should take that in a follow-up, see the filed bug for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Fun"
No description provided.