sentry-javascript icon indicating copy to clipboard operation
sentry-javascript copied to clipboard

fix(node): Remove deprecated `routerPath` warning of fastify request object

Open Zen-cronic opened this issue 1 year ago • 5 comments

Fixes #12844

For the tests, would dev-packages/node-integration-tests/suites or dev-packages/e2e-tests/test-applications/node-fastify be a more appropriate location?

I also noticed that there's currently no test for fastify in node-integration.

  • [x] If you've added code that should be tested, please add tests.
  • [x] Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

Zen-cronic avatar Jul 24 '24 17:07 Zen-cronic

About the test, we can spyOn/mock the process.emitWarning() (which fastify uses), and assert that it's never invoked. But the test runner uses Axios, which throws its own error when faced with a 404. This happens before reaching the fastify code. So mocking would have no effect.

Although the deprecation warning is not logged (when manually tested with the fixed code), I'm stuck at including the case into the test suite. Currently, only the AxiosError is tested. Any thoughts?

Zen-cronic avatar Jul 25 '24 17:07 Zen-cronic

I'm stuck at including the case into the test suite. Currently, only the AxiosError is tested

We should probably move away from Axios in general, I think this limitation is fine because we know the implementation works in OTEL.

Let's just leave a comment on the test about the deprecation warning not being logged.

AbhiPrasad avatar Jul 31 '24 15:07 AbhiPrasad

After we add a comment to the test, and rebase this PR to latest, it's good to go to merge in my eyes.

Thanks for the PR @Zen-cronic (also nice to see some Canadian contributions 🇨🇦)

Happy to help :)

Also I believe my rebase works correctly?

Edit: I think I messed up.

Zen-cronic avatar Aug 02 '24 01:08 Zen-cronic

assigning myself to help get this merged in!

AbhiPrasad avatar Aug 02 '24 16:08 AbhiPrasad

Thanks for helping out!

Zen-cronic avatar Aug 06 '24 15:08 Zen-cronic

@AbhiPrasad can this be merged?

andreiborza avatar Dec 10 '24 16:12 andreiborza

yeah let me rebase and try to get this to the finish line

AbhiPrasad avatar Dec 10 '24 16:12 AbhiPrasad

This is superseded by https://github.com/getsentry/sentry-javascript/pull/15542 - thank you for the effort on this!

mydea avatar Apr 23 '25 07:04 mydea