test(replay): Add integration test for privacy #7055
Conversation
size-limit report 📦
|
| 'childNodes': [ | ||
| { | ||
| 'id': 16, | ||
| 'textContent': 'This should be unmasked due to data attribute', |
There was a problem hiding this comment.
This should break when we merge and release getsentry/rrweb#40
packages/replay/src/index.ts
Outdated
| @@ -1 +1,3 @@ | |||
| export { Replay } from './integration'; | |||
|
|
|||
| export type {RecordingEvent} from './types' | |||
There was a problem hiding this comment.
Wasn't sure the best way to expose this type for the test
There was a problem hiding this comment.
Yeah I was also struggling with this and opted to duplicate the type in #7052. If we're comfortable exposing this as public API, I think exporting it is fine and I'll also switch to it in my PR.
The one reason against exporting it would be breaking changes in rrweb 2.x, if any? But considering how far off it seems that we're upgrading, I'd say it's negligible. WDYT?
billyvg
changed the title
packages/replay/src/index.ts
Outdated
| @@ -1 +1,3 @@ | |||
| export { Replay } from './integration'; | |||
|
|
|||
| export type {RecordingEvent} from './types' | |||
There was a problem hiding this comment.
Yeah I was also struggling with this and opted to duplicate the type in #7052. If we're comfortable exposing this as public API, I think exporting it is fine and I'll also switch to it in my PR.
The one reason against exporting it would be breaking changes in rrweb 2.x, if any? But considering how far off it seems that we're upgrading, I'd say it's negligible. WDYT?
| @@ -17,8 +17,8 @@ export const envelopeParser = (request: Request | null): unknown[] => { | |||
| }); | |||
| }; | |||
|
|
|||
| export const envelopeRequestParser = (request: Request | null): Event => { | |||
| return envelopeParser(request)[2] as Event; | |||
| export const envelopeRequestParser = <T = Event>(request: Request | null, index = 2): T => { | |||
| @@ -0,0 +1,18 @@ | |||
| import * as Sentry from '@sentry/browser'; | |||
| import { Replay } from '@sentry/replay'; | |||
There was a problem hiding this comment.
Huh, so these tests were breaking from es6 bundles when I did not use Replay from @sentry/replay (e.g. I was usin Sentry.Replay). Any ideas @Lms24?
There was a problem hiding this comment.
Yup, this was how we currently still differ between injecting the addon replay CDN bundle in addition to the CDN bundle (w/o replay) vs. testing the NPM @sentry/browser export.
I'm trying to get #7096 merged today which will get rid of relying on imports here. Going forward, we'll drop testing the addon bundle but instead additionally test against the two bundle variants that include Replay directly.
| @@ -0,0 +1,236 @@ | |||
| { | |||
There was a problem hiding this comment.
WDYT about using snapshots here @Lms24 -- It's cumbersome trying to update these by hand.
There was a problem hiding this comment.
I initially thought it'd be good to use snapshots in such situations. In #7082, it turned out though, that Playwright needs to have a snapshot for each platform+browser combination, meaning we'd likely end up with 12+ snapshot files per snapshot. The problem here is that this temporarily breaks tests on CI as well as for other folks running them locally and they'd need to contribute their newly made snapshots to the repo. We opted to therefore stay away from snapshots.
There was a problem hiding this comment.
@Lms24 Yeah I noticed that too, I was able to find a way to disable the platform, so now it's only by browser. This makes it possible to update snapshots locally, you just have to run --update-snapshots with --browser="all". Does that make it more reasonable?
There was a problem hiding this comment.
Removing snapshots for now so I can merge this in now.
There was a problem hiding this comment.
Oh nice, didn't know this worked. Then sure, snapshots would be okay, too. Actually that's good to know for my tests as well!
There was a problem hiding this comment.
👍 I'll make a new PR for it
Adds an integration test for default privacy settings.