-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
chore: add --changedSince base SHA to Jest in CI #110568
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
Changes from all commits
7994694
b5c9931
7e2b9e2
5c9ae7e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| const SentryEnvironment = require('@sentry/jest-environment/jsdom'); | ||
|
|
||
| // @sentry/jest-environment mutates config.projectConfig.testEnvironmentOptions | ||
| // .sentryConfig.init in-place (pushing integrations and calling Sentry.init). | ||
| // When Jest runs in-band (≤1 test, e.g. via --changedSince), those mutations | ||
| // create circular references that crash ScriptTransformer's config serialisation. | ||
| // Deep-cloning sentryConfig isolates the mutation from the original config object. | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Question] Is there any context around Example where it crashed from running in-band: https://github.com/getsentry/sentry/actions/runs/23297657610/job/67749510053?pr=110624 Aside: I noticed https://www.npmjs.com/package/@sentry/jest-environment was last published 9 months ago for 6.1.0, but the linked repo https://github.com/billyvg/jest-sentry-environment was last committed 5 years ago for 1.3.0 and is archived. https://github.com/getsentry/jest-sentry-environment is the new place.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| class SafeSentryEnvironment extends SentryEnvironment { | ||
| /** @param {import('@jest/environment').JestEnvironmentConfig} config @param {import('@jest/environment').EnvironmentContext} context */ | ||
| constructor(config, context) { | ||
| const sentryConfig = config.projectConfig.testEnvironmentOptions?.sentryConfig; | ||
| if (sentryConfig) { | ||
|
Comment on lines
+8
to
+12
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'd try to add these to https://github.com/getsentry/jest-sentry-environment and publish a new version
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| config = { | ||
| ...config, | ||
| projectConfig: { | ||
| ...config.projectConfig, | ||
| testEnvironmentOptions: { | ||
| ...config.projectConfig.testEnvironmentOptions, | ||
| sentryConfig: structuredClone(sentryConfig), | ||
| }, | ||
| }, | ||
| }; | ||
| } | ||
| super(config, context); | ||
| } | ||
| } | ||
|
|
||
| module.exports = SafeSentryEnvironment; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| declare module '@sentry/jest-environment/jsdom' { | ||
| // eslint-disable-next-line import/no-extraneous-dependencies -- transitive dep of jest | ||
| import type {JestEnvironment} from '@jest/environment'; | ||
|
|
||
| const SentryEnvironment: typeof JestEnvironment; | ||
| export = SentryEnvironment; | ||
| } |
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.
Shard balancing uses unfiltered total, breaking distribution
Medium Severity
speculatedSuiteDurationis computed from the fulltestStats(all ~1,861 suites), but the new filtering at lines 143–146 correctly restricts thetestsMap to only theallTestssubset from--changedSince. SincetargetDurationis massively inflated relative to the actual filtered tests' total duration, the balancing loop stuffs every test into the first shard while shards 1–3 get nothing. This defeats parallelization for any small-to-medium--changedSinceresult set.Additional Locations (1)
jest.config.ts#L139-L146There 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, interesting. I don't think parallelization at those result set scales really matters much, right? Running 10 suites vs 100 is pretty quick.