-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix(core): Ensure global event processors are always applied to event #9064
Conversation
Even if no scope is passed.
@@ -15,7 +15,7 @@ for (const dir of scenariosDirs) { | |||
|
|||
const processes = scenarios.map(([filename, filepath]) => { | |||
return new Promise(resolve => { | |||
const scenarioProcess = spawn('node', [filepath]); | |||
const scenarioProcess = spawn('node', [filepath], { timeout: 10000 }); |
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.
Adding a timeout so this does not run forever, if a failure happens.
@@ -12,6 +12,9 @@ function cleanUpAndExitSuccessfully() { | |||
} | |||
|
|||
function assertSessionAggregates(session, expected) { | |||
if (!session.aggregates) { |
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.
this already failed before, but did not result in test failures because apparently some global event processors were not running and thus no sentry error was produced for this (?)
size-limit report 📦
|
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.
Hmm generally, I think this is fine and fixes broken behaviour. I'm just wondering when there's actually no scope available... shouldn't there at least be the "base" scope that's created alongside the hub?
This happens e.g. if you call |
Even if no scope is passed.
I guess this is kind of behavior changing, but I'd argue it is just fixing an unexpected behaviour that's not actually intuitive/logical - that global event processors are only applied if a scope is provided.
I extracted this out from #9032, as I figured this is actually a reasonable change on it's own, so it's easier to reason about this in it's own PR.