Hubs/Scopes Merge 40 - Scopes.isEnabled now checks getClient().isEnabled()
#3385
Conversation
…pes-merge-2-add-scopes
…ainScopes to rootScopes
adinauer
requested review from
romtsn,
stefanosiano and
markushi
as code owners
|
Performance metrics 🚀
|
| val globalScope = Scope(options) | ||
| val scopes = Scopes(mock<IScope>(), mock<IScope>(), globalScope, "test") | ||
| // verify(integrationMock).register(expected, options) | ||
| val scopes = createTestScopes(options) |
There was a problem hiding this comment.
Extracted this into test-support so we can reuse it and not have to set up mocks every time.
| @@ -82,17 +89,25 @@ class SentryAppenderTest { | |||
|
|
|||
| @Test | |||
| fun `does not initialize Sentry if Sentry is already enabled`() { | |||
There was a problem hiding this comment.
afaict this test didn't actually test what it says it does. It used to just initialize Sentry via the log appender then reinitialize using Sentry.init and it asserted that the env from re-init was set.
| @@ -10,14 +11,14 @@ public final class EventProcessorUtils { | |||
|
|
|||
| public static List<EventProcessor> unwrap( | |||
| final @Nullable List<EventProcessorAndOrder> orderedEventProcessor) { | |||
There was a problem hiding this comment.
Maybe rename to eventProcessorsWithOrder, same as in Scope.java to make it clear that these are not ordered, but come with an order.
There was a problem hiding this comment.
Will do in a follow up PR
| @@ -2166,6 +2175,24 @@ class ScopesTest { | |||
| assertEquals(span.spanContext.parentSpanId, txn.spanContext.spanId) | |||
| } | |||
|
|
|||
| @Test | |||
| fun `is considered enabled if client is enabled()`() { | |||
| val scopes = generateScopes() as Scopes | |||
There was a problem hiding this comment.
Cast to Scopes is not needed
There was a problem hiding this comment.
Will change in a follow up PR
| } | ||
|
|
||
| @Test | ||
| fun `is considered disabled if client is disabled()`() { |
There was a problem hiding this comment.
Cast to Scopes is not needed
There was a problem hiding this comment.
Will change in a follow up PR
#skip-changelog
📜 Description
💡 Motivation and Context
Since
Scopesare forked much more frequently thanHubwas, we can no longer storeisEnabledas a property onScopesas it would only affect a very limited scope (:D). By asking client we should by default now have the same state globally unless a customer sets a different client.💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps