sentry-javascript
sentry-javascript copied to clipboard
fix(node): Remove deprecated `routerPath` warning of fastify request object
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).
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?
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.
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.
assigning myself to help get this merged in!
Thanks for helping out!
@AbhiPrasad can this be merged?
yeah let me rebase and try to get this to the finish line
This is superseded by https://github.com/getsentry/sentry-javascript/pull/15542 - thank you for the effort on this!