feat(effect): Add E2E tests for the @sentry/effect SDK#19763
feat(effect): Add E2E tests for the @sentry/effect SDK#19763JPeer264 merged 3 commits intojp/add-effect-sdkfrom
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| console.error(err.details); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
Build script silently swallows webpack fatal errors
Medium Severity
When webpack invokes the callback with a fatal err (e.g. configuration error or file system failure — distinct from compilation errors), the handler logs the error and calls return without process.exit(1). The build process then exits with code 0, making CI falsely report a successful build even though no output was produced, causing the subsequent pnpm start to serve stale or missing files.
| const contextLog = logs.find(log => log.body === 'Log with context'); | ||
| expect(contextLog).toBeDefined(); | ||
| expect(contextLog?.level).toBe('info'); | ||
| }); |
There was a problem hiding this comment.
Log context test never verifies context attributes
Low Severity
Both "should send Effect logs with context attributes" tests use Effect.annotateLogs('userId', '12345') and Effect.annotateLogs('action', 'test') to attach context, but the assertions only check body and level. The userId and action attributes are never asserted, so the test doesn't actually verify the behaviour its name describes. A regression where context attributes stop being forwarded would go undetected.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
size-limit report 📦
|
JPeer264
force-pushed
the
jp/add-effect-sdk-stack/6
branch
from
1e82465 to
5c2a44e
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| const contextLog = logs.find(log => log.body === 'Log with context'); | ||
| expect(contextLog).toBeDefined(); | ||
| expect(contextLog?.level).toBe('info'); | ||
| }); |
There was a problem hiding this comment.
Log context test doesn't assert context attributes
Medium Severity
The tests named "should send Effect logs with context attributes" never assert that userId or action attributes are present in the serialized log payload. They only check level and body. The app code uses Effect.annotateLogs('userId', '12345') and Effect.annotateLogs('action', 'test'), but SentryEffectLogger in logger.ts only destructures { logLevel, message } from the Effect Logger callback — annotations are silently ignored and never forwarded to sentryLogger. These tests give false confidence that context attributes work when they're actually dropped. The test names claim to verify context attributes but the assertions don't cover them.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
JPeer264
force-pushed
the
jp/add-effect-sdk
branch
2 times, most recently
from
2db4b85 to
3070535
Compare
JPeer264
force-pushed
the
jp/add-effect-sdk-stack/6
branch
from
5c2a44e to
e532d18
Compare
| @@ -0,0 +1,101 @@ | |||
| import * as Sentry from '@sentry/effect/server'; | |||
There was a problem hiding this comment.
q: Actually, can we not make this isomorphic? Is there a reason to not support both client/server from @sentry/effect?
There was a problem hiding this comment.
It is working already without /server. I'll update it
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| if (err) { | ||
| console.error(err.stack || err); | ||
| if (err.details) { | ||
| console.error(err.details); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
Bug: The webpack build script logs fatal configuration errors but doesn't exit with a failure code, causing the CI build to appear successful despite the failure.
Severity: MEDIUM
Suggested Fix
Add process.exit(1) within the if (err) block after logging the error. This will ensure that any fatal webpack configuration error causes the build process to fail with a non-zero exit code, correctly signaling a build failure in the CI pipeline.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: dev-packages/e2e-tests/test-applications/effect-browser/build.mjs#L32-L38
Potential issue: In the webpack build script, if a fatal configuration error occurs, the
`err` object is logged, but the process does not exit with a non-zero status code.
Instead, it returns and exits cleanly with code 0. This causes the CI/CD pipeline to
incorrectly report the build as successful. Subsequent steps, like starting the
application server, will then fail with a confusing error because the build artifacts
were never created. This masks the true root cause of the failure, which is the webpack
configuration error.
Did we get this right? 👍 / 👎 to inform future reviews.
2 participants
This adds Node and Browser tests for the
@sentry/effectSDK.I am not sure what to do with the browser part, as there is I guess no tree-shaking available right now.
The basic usage for node and browser are the exact same, only the
effectLayerhas to be added into the runtime layer.