Fix/nestjs cron issues #16792
Fix/nestjs cron issues #16792
Conversation
Add a command to delete the Sentry-related cache during the E2E test setup process. This ensures a clean testing environment and prevents potential issues caused by stale cache. This is why running tests locally after any change might be wrong. Not sure this is the best solution, or no. But, at least results are constant now
Introduced a new `testSyncCronError` method to handle sync cron job errors and updated the async method for clarity. Updated tests to cover both async and sync cron scenarios, ensuring proper error handling and reporting to Sentry.
Added error capturing for both synchronous and asynchronous exceptions in monitored methods. Incorporated `isThenable` checks to handle promises properly, ensuring robust error tracking with Sentry in both cases.
There was a problem hiding this comment.
This looks good to me, thank you for contributing this!
cc @chargome
| @@ -66,6 +66,7 @@ async function run(): Promise<void> { | |||
| } | |||
|
|
|||
| await asyncExec('pnpm clean:test-applications', { env, cwd: __dirname }); | |||
| await asyncExec('pnpm cache delete "@sentry/*"', { env, cwd: __dirname }); | |||
There was a problem hiding this comment.
m: Is this necessary? Did you run into issues without it? I think we should not add this in this PR.
There was a problem hiding this comment.
To be honest - no, it is not necessary to be here in this PR. But yes, you are going to run into an issue without this. The issue is that if you are doing any changes to any package and want to run e2e tests locally with yarn workspace @sentry-internal/e2e-tests test:run app-name your very first test will be executed as expected. But any other changes are not going to be reflected by pnpm install because of the cache.
There was a problem hiding this comment.
I would also rather get rid of this and if necessary address this in a different PR.
Added JSDoc callback refference Removed redundant else in if-else block
Updated the test to safely handle potential undefined values in the stacktrace frames. This prevents runtime errors during test execution and ensures robust validation of error handling logic.
…16749 Introduce a new endpoint `/crash-in-with-monitor/:id` that demonstrates error handling using Sentry's `withMonitor` API. Added tests to ensure the app gracefully handles exceptions and validates consistent PID behavior during monitoring.
|
Added tests to cover #16749 and did some refactoring in |
dev-packages/e2e-tests/test-applications/node-express/tests/errors.test.ts
Outdated
Show resolved
Hide resolved
| @@ -66,6 +66,7 @@ async function run(): Promise<void> { | |||
| } | |||
|
|
|||
| await asyncExec('pnpm clean:test-applications', { env, cwd: __dirname }); | |||
| await asyncExec('pnpm cache delete "@sentry/*"', { env, cwd: __dirname }); | |||
There was a problem hiding this comment.
I would also rather get rid of this and if necessary address this in a different PR.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Unrelated Cache Command in PR
The pnpm cache delete "@sentry/*" command was committed to dev-packages/e2e-tests/run.ts. This command is unrelated to the PR's main purpose of fixing NestJS cron issues, though it was intended to address a separate local development caching problem. Despite PR discussion consensus among reviewers and the author that it was unnecessary for this PR and should be handled separately, it remains in the committed code.
dev-packages/e2e-tests/run.ts#L68-L69
sentry-javascript/dev-packages/e2e-tests/run.ts
Lines 68 to 69 in 383ae52
Was this report helpful? Give feedback by reacting with 👍 or 👎
|
@0xbad0c0d3 thanks again for these contributions, if you'd like a free Sentry T-shirt please email me at my Github username -> firstname.lastname at sentry.io :) |
This PR adds the external contributor to the CHANGELOG.md file, so that they are credited for their contribution. See #16792 Co-authored-by: andreiborza <168741329+andreiborza@users.noreply.github.com>
Nestjs sync task has the same issue as async tasks - they don't care about exceptions because nestjs
@Cron()decorator automatically wraps it totry-catchblock and logs an error. That's why we should capture exception in our@SentryCron()decorator.Fixes also #16749
yarn lint) & (yarn test).