-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(remix): Ensure source maps upload fails silently if Sentry CLI fails #17082
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
base: develop
Are you sure you want to change the base?
Conversation
b230de2
to
894f638
Compare
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.
Bug: Test Pollution from Persistent Mock States
Tests suffer from isolation issues because the globally created consoleWarnSpy
is not cleared in beforeEach
, causing its call history to persist. Additionally, mockRejectedValue()
implementations for uploadSourceMapsMock
and finalizeMock
are not reset between tests, leading to their rejection behavior persisting. These omissions cause test pollution and unreliable results.
packages/remix/test/scripts/upload-sourcemaps.test.ts#L7-L46
const consoleWarnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); | |
// The createRelease script requires the Sentry CLI, which we need to mock so we | |
// hook require to do this | |
async function mock(mockedUri: string, stub: any) { | |
const { Module } = await import('module'); | |
// @ts-expect-error test | |
Module._load_original = Module._load; | |
// @ts-expect-error test | |
Module._load = (uri, parent) => { | |
if (uri === mockedUri) return stub; | |
// @ts-expect-error test | |
return Module._load_original(uri, parent); | |
}; | |
} | |
await vi.hoisted(async () => | |
mock( | |
'@sentry/cli', | |
vi.fn().mockImplementation(() => { | |
return { | |
execute: vi.fn(), | |
releases: { | |
new: newMock, | |
uploadSourceMaps: uploadSourceMapsMock, | |
finalize: finalizeMock, | |
proposeVersion: proposeVersionMock, | |
}, | |
}; | |
}), | |
), | |
); | |
// eslint-disable-next-line @typescript-eslint/no-var-requires | |
const { createRelease } = require('../../scripts/createRelease'); | |
beforeEach(() => { | |
newMock.mockClear(); | |
uploadSourceMapsMock.mockClear(); |
packages/remix/test/scripts/upload-sourcemaps.test.ts#L100-L124
sentry-javascript/packages/remix/test/scripts/upload-sourcemaps.test.ts
Lines 100 to 124 in 8e474ed
it('logs an error when uploadSourceMaps fails', async () => { | |
uploadSourceMapsMock.mockRejectedValue(new Error('Failed to upload sourcemaps')); | |
await createRelease({}, '~/build/', 'public/build'); | |
expect(uploadSourceMapsMock).toHaveBeenCalledWith('0.1.2.3.4', { | |
urlPrefix: '~/build/', | |
include: ['public/build'], | |
useArtifactBundle: true, | |
live: 'rejectOnError', | |
}); | |
expect(consoleWarnSpy).toHaveBeenCalledWith('[sentry] Failed to upload sourcemaps.'); | |
expect(finalizeMock).toHaveBeenCalledWith('0.1.2.3.4'); | |
}); | |
it('logs an error when finalize fails', async () => { | |
finalizeMock.mockRejectedValue(new Error('Failed to finalize release')); | |
await createRelease({}, '~/build/', 'public/build'); | |
expect(consoleWarnSpy).toHaveBeenCalledWith('[sentry] Failed to finalize release.'); | |
}); |
Was this report helpful? Give feedback by reacting with 👍 or 👎
Sentry CLI's
cli.releases.uploadSourceMaps
method previously never rejected when the actual CLI binary execution exited with an error code. In CLI 2.49.0 (and 2.50.0) I added a new live moderejectOnError
which continues to pipe stdio to the process (the remix SDKs' upload script) but now also rejects on error.This PR 1. bumps Sentry CLI, 2. configures the CLI to actually reject now but 3. also catches the rejection and logs a message. I decided to still continue with the script because we should still delete source maps. Otherwise, we risk deploying them when users expect them to be deleted. (i.e. fail silently but correctly :D)