Repository navigation
Conversation
61465
requested review from
nicohrubec and
s1gr1d
and removed request for
a team
October 14, 2026 03:25
Author
|
Just rebased onto Two shapes available — happy with either:
Shape 1 is lower churn. Shape 2 reduces test file count by one. Which do you prefer? |
Contributor
|
👋 @nicohrubec, @s1gr1d — Please review this PR when you get a chance! |
1 similar comment
Contributor
|
👋 @nicohrubec, @s1gr1d — Please review this PR when you get a chance! |
Fills in the Nuxt client-plugin unit tests requested in getsentry#13813. Supersedes the dormant draft in getsentry#24583 (last activity 2026-02; author pinged 2026-10-12 by @JPeer264 with no reply). Covers the five code paths in `src/runtime/plugins/sentry.client.ts`: 1. Vue route provider setup (installs, respects user provider, skips without client, skips without `$router`) 2. `browserTracingIntegration` (adds with router+path label, skips on no client, skips on no router) 3. `app:created` hook — `vueIntegration` with `attachErrorHandler: false`, including client-torn-down path 4. `app:error` hook — 4xx/3xx status-code skip for Nuxt errors, 5xx and non-Nuxt errors still reported 5. `vue:error` hook — forwards error + instance + info to `reportNuxtError` Follows the mock pattern already used in `captureErrorHook.test.ts` (vi.hoisted + vi.mock(importOriginal) + spyOn on local utils). Pure vitest, no native deps. Refs getsentry#13813. Signed-off-by: Abd Alrahaman <abdarahman10555@gmail.com>
61465
force-pushed
the
fix/nuxt-client-plugin-tests
branch
from
October 22, 2026 11:23
49070f6 to
be38a35
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit be38a35. Configure here.
…#25348 Three concrete gaps against the suite this PR replaced: - Nuxt-error status filter now also exercises 3xx (301), not just 4xx. Both `app:error` and `vue:error` use `it.each([404, 301])` to match upstream getsentry#25341's pattern. - The "non-Nuxt error" case now carries a 4xx statusCode on the error, which is what actually exercises the `isNuxtError` guard — without the guard the error would be skipped as 4xx. Previous shape passed a plain Error with no statusCode, so the guard was not actually under test. - Added the equivalent non-Nuxt-with-statusCode case for `vue:error` (previously only present under `app:error`). No logic change in the plugin — just test coverage. Signed-off-by: Abd Alrahaman <abdarahman10555@gmail.com>
logaretm
requested changes
Oct 26, 2026
logaretm
left a comment
logaretm
left a comment
Member
There was a problem hiding this comment.
Thanks for picking this up, the tests do pass but I think we should change a few things before this goes in.
Comment on lines
+70
to
+73
| expect(createVueRouteProvider).toHaveBeenCalledTimes(1); | ||
| // The provider is a lazy router getter — confirm it resolves to our router. | ||
| const routerGetter = createVueRouteProvider.mock.calls[0]![0] as () => unknown; | ||
| expect(routerGetter()).toBe(router); |
Member
There was a problem hiding this comment.
Can you use this here instead of reading mock.calls?
Suggested change
| expect(createVueRouteProvider).toHaveBeenCalledTimes(1); | |
| // The provider is a lazy router getter — confirm it resolves to our router. | |
| const routerGetter = createVueRouteProvider.mock.calls[0]![0] as () => unknown; | |
| expect(routerGetter()).toBe(router); | |
| expect(createVueRouteProvider).toHaveBeenCalledWith(expect.any(Function)); |
I don't think we need to check that the getter returns the router.
Comment on lines
+159
to
+165
| expect(vueIntegration).toHaveBeenCalledTimes(1); | ||
| const vueCallArgs = vueIntegration.mock.calls[0]![0] as { | ||
| app: unknown; | ||
| attachErrorHandler: boolean; | ||
| }; | ||
| expect(vueCallArgs.app).toBe(vueApp); | ||
| expect(vueCallArgs.attachErrorHandler).toBe(false); |
Member
There was a problem hiding this comment.
Same here:
Suggested change
| expect(vueIntegration).toHaveBeenCalledTimes(1); | |
| const vueCallArgs = vueIntegration.mock.calls[0]![0] as { | |
| app: unknown; | |
| attachErrorHandler: boolean; | |
| }; | |
| expect(vueCallArgs.app).toBe(vueApp); | |
| expect(vueCallArgs.attachErrorHandler).toBe(false); | |
| expect(vueIntegration).toHaveBeenCalledWith(expect.objectContaining({ app: vueApp, attachErrorHandler: false })); |
|
|
||
| vi.mock(import('@sentry/core'), async importOriginal => { | ||
| const mod = await importOriginal(); | ||
| return { ...mod, getClient, GLOBAL_OBJ: {} as never }; |
Member
There was a problem hiding this comment.
Since GLOBAL_OBJ is stubbed to {}, we never test that _sentryNuxtVueIntegrationOptions gets passed to vueIntegration. That's the Nuxt-specific part here, can you add a test for it?
| it('reports errors that are not Nuxt errors from vue:error', async () => { | ||
| const hook = await setupHook('vue:error'); | ||
| const error = Object.assign(new Error('boom'), { statusCode: 404 }); | ||
| describe('vue:error hook', () => { |
Member
There was a problem hiding this comment.
These are mostly copies of the app:error tests. Can we go back to it.each(['app:error', 'vue:error']) like before and only keep the instance/info test separate?
Four review items from the client.test.ts pass:
1. Replace mock.calls read with an expect.toHaveBeenCalledWith(expect.any(Function))
assertion for the createVueRouteProvider router-getter case.
2. Mock GLOBAL_OBJ with _sentryNuxtVueIntegrationOptions so the Nuxt-specific
forwarding path into vueIntegration is actually covered. Previously
GLOBAL_OBJ was stubbed to {} and the merge was invisible.
3. Replace vueIntegration mock.calls read with
expect.toHaveBeenCalledWith(expect.objectContaining({...})), and assert
the GLOBAL_OBJ options flow through.
4. Collapse the duplicated status-code tests on app:error and vue:error
into one describe.each(['app:error', 'vue:error']) via a small fireHook
helper. Keep the vue:error-specific instance/info passthrough test as
its own describe.
Local run (packages/nuxt): 18/18 passed, sentry.client.ts coverage 95.83%.
Signed-off-by: Abd Alrahaman <abdarahman10555@gmail.com>
Author
|
@logaretm — thanks for the pass. Pushed
Local: 18/18 pass, |
s1gr1d
reviewed
Oct 27, 2026
Member
There was a problem hiding this comment.
This file has CRLF line endings, please format the PR first (also fails in CI).
Comment on lines
+210
to
+219
| it.each([404, 301])('skips reporting when a Nuxt error carries a %d status code', async statusCode => { | ||
| const { isNuxtError } = await import('nuxt/app'); | ||
| const reportSpy = vi.spyOn(utils, 'reportNuxtError').mockImplementation(() => {}); | ||
| vi.mocked(isNuxtError).mockReturnValue(true); | ||
| getClient.mockReturnValue(makeClient()); | ||
| getRouteProvider.mockReturnValue(undefined); | ||
|
|
||
| const nuxtApp = makeNuxtApp(); | ||
| await (clientPlugin as NuxtPluginShape).setup(nuxtApp); | ||
| fireHook(nuxtApp, { statusCode }); |
Member
There was a problem hiding this comment.
isNuxtError checks both status and statusCode. Make sure to test both.
e.g. with this:
it.each([
{ status: 404 },
{ status: 301 },
{ statusCode: 404 },
{ statusCode: 301 },
])('skips reporting when a Nuxt error carries %o', ...)…elds
Two review items from s1gr1d:
1. The client.test.ts file I pushed had CRLF line endings from a
Windows checkout with core.autocrlf=true. Converted to LF and
staged with core.autocrlf=false so the file stays consistent with
the rest of packages/nuxt.
2. isNuxtError on a Nuxt error can reach the plugin's skip guard via
either error.status or error.statusCode (see
sentry.client.ts:45). The previous it.each only covered the
statusCode branch. Expanded to four cases:
{status: 404} / {status: 301} / {statusCode: 404} / {statusCode: 301}
using %o formatting so the test name reflects the shape under test.
Local run (packages/nuxt): 22/22 passed, sentry.client.ts coverage
unchanged at 95.83%.
Signed-off-by: Abd Alrahaman <abdarahman10555@gmail.com>
Author
|
@s1gr1d — thanks for the pass. Pushed
Local: 22/22 pass, |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #13813.
Fills in the Nuxt client-plugin unit tests requested by @mydea. Picks up where the dormant draft #24583 (@vansh-nagar, inactive since 2026-02; pinged by @JPeer264 on 2026-10-12 with no reply within the stated window) left off and extends it to all five code paths in
packages/nuxt/src/runtime/plugins/sentry.client.ts.What this covers
_INTERNAL_createVueRouteProvider→setRouteProvider)getClient()returns a client,$routeris on the Nuxt app, and no provider is set$routeron the Nuxt appbrowserTracingIntegration{ router, routeLabel: 'path' }on the happy path$routerapp:createdhook — Vue integrationvueIntegrationwithattachErrorHandler: falseand the live Vue appgetClient()returnsundefined)app:errorhook — status-code filteringvue:errorhook{ error, instance, info }toreportNuxtErrorPattern
Mocks mirror
packages/nuxt/test/runtime/hooks/captureErrorHook.test.ts:vi.hoisted(() => vi.fn())for cross-module mock refsvi.mock(import('…'), async importOriginal => …)for@sentry/core,@sentry/vue,nuxt/appvi.spyOn(utils, 'reportNuxtError')for the local../utilsmodulePure vitest, no native dependencies. Runs under the existing
yarn testcommand inpackages/nuxt/.Local verification
Syntax parse via
tsc --noEmit --skipLibCheckon the test file passes (only "Cannot find module" warnings remain for workspace packages, which CI resolves viayarn install). I was unable to materialise the Yarn/npm workspace node_modules locally on my Windows + Node 24 shell (ETARGET onwhatwg-url@^17.2.0, same error reported upstream in jsdom's current chain), so I'm relying on your CI matrix to confirm the suite is green across the vitest + jsdom environment configured inpackages/nuxt/vite.config.ts.Notes
src/runtime/plugins/sentry.client.ts; happy to re-shape the test file structure if the preferred house style here is onedescribeper public behaviour rather than per source-code region.cc @mydea + @JPeer264 — thanks for the pre-blessing on #13813 and for keeping the issue open.
🤖 Used Claude Code for the test scaffolding and to cross-check the mock shapes against existing nuxt tests; every assertion was signed off by me on content.