Skip to content

test(replay): Test that blocking elements works as expected #7201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 20, 2023

Conversation

mydea
Copy link
Member

@mydea mydea commented Feb 16, 2023

This adds a test to verify that blocking elements manually works as expected. Esp. also blocking stuff in the head tag.

(Note that it does, this is just to ensure it stays that way).

See #7184

@mydea mydea requested a review from billyvg February 16, 2023 12:16
@mydea mydea self-assigned this Feb 16, 2023
@github-actions
Copy link
Contributor

Replay SDK metrics 🚀

    Plain +Sentry +Replay
Revision Value Value Diff Ratio Value Diff Ratio
LCP This PR a8449de 72.54 ms 95.15 ms +22.61 ms +31.17 % 130.81 ms +58.27 ms +80.34 %
Previous 79babe9 72.40 ms 93.06 ms +20.66 ms +28.54 % 131.09 ms +58.69 ms +81.06 %
CLS This PR a8449de 0.06 ms 0.06 ms +0.00 ms +0.37 % 0.06 ms -0.00 ms -0.44 %
Previous 79babe9 0.06 ms 0.06 ms -0.00 ms -0.01 % 0.06 ms -0.00 ms -0.45 %
CPU This PR a8449de 14.51 % 16.48 % +1.98 pp +13.64 % 21.63 % +7.12 pp +49.08 %
Previous 79babe9 13.12 % 12.08 % -1.04 pp -7.92 % 17.52 % +4.40 pp +33.50 %
JS heap avg This PR a8449de 1.94 MB 1.99 MB +48.65 kB +2.50 % 2.87 MB +927.42 kB +47.73 %
Previous 79babe9 1.94 MB 1.99 MB +47 kB +2.42 % 2.87 MB +927.46 kB +47.71 %
JS heap max This PR a8449de 2.3 MB 2.55 MB +249.36 kB +10.83 % 3.36 MB +1.05 MB +45.74 %
Previous 79babe9 2.3 MB 2.56 MB +258.01 kB +11.20 % 3.36 MB +1.06 MB +46.03 %
netTx This PR a8449de 0 B 0 B 0 B n/a 2.2 kB +2.2 kB n/a
Previous 79babe9 0 B 0 B 0 B n/a 2.23 kB +2.23 kB n/a
netRx This PR a8449de 0 B 0 B 0 B n/a 41 B +41 B n/a
Previous 79babe9 0 B 0 B 0 B n/a 41 B +41 B n/a
netCount This PR a8449de 0 0 0 n/a 1 +1 n/a
Previous 79babe9 0 0 0 n/a 1 +1 n/a
netTime This PR a8449de 0.00 ms 0.00 ms 0.00 ms n/a 98.31 ms +98.31 ms n/a
Previous 79babe9 0.00 ms 0.00 ms 0.00 ms n/a 103.20 ms +103.20 ms n/a

Previous results on branch: develop

RevisionLCPCLSCPUJS heap avgJS heap maxnetTxnetRxnetCountnetTime
79babe9+58.69 ms-0.00 ms+4.40 pp+927.46 kB+1.06 MB+2.23 kB+41 B+1+103.20 ms
5359ba9+55.62 ms-0.00 ms+4.29 pp+935.26 kB+1.05 MB+2.2 kB+41 B+1+79.05 ms

*) pp - percentage points - an absolute difference between two percentages.
Last updated: Thu, 16 Feb 2023 12:21:31 GMT

@billyvg
Copy link
Member

billyvg commented Feb 16, 2023

@mydea FWIW I just ran yarn test:update-snapshots in the repo root and it renders as 18px on my computer

@mydea
Copy link
Member Author

mydea commented Feb 20, 2023

OK, now I re-ran it and it is correct again. Maybe a chrome update or something like this...

@mydea mydea force-pushed the fn/replay-test-block branch from 146a0ca to 6bf9653 Compare February 20, 2023 08:50
@@ -0,0 +1 @@
test-results
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a .gitginore to ensure this is not accidentally checked in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, also saw this folder floating around recently

@mydea mydea requested a review from Lms24 February 20, 2023 08:58
@mydea
Copy link
Member Author

mydea commented Feb 20, 2023

Hmm, now I get some other failures due to different value rounding of attributes 😬 TBH not quite sure how to make this more robust...

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally this looks good to me but I have some thoughts around getting the snapshot and normalizing it.

@@ -0,0 +1 @@
test-results
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, also saw this folder floating around recently

Comment on lines 27 to 30
const replayPayload = envelopeRequestParser<RecordingEvent[]>(await reqPromise0, 5);
const checkoutEvent = replayPayload.find(({ type }) => type === EventType.FullSnapshot);

expect(JSON.stringify(checkoutEvent?.data, null, 2)).toMatchSnapshot('privacy.json');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m/h: Two things here:

  • replayHelpers.ts has two functions, we can use to obtain the snapshot without having to manually call the envelope parser. This will also ensure that getting the snapshot works if we enabled compression in init.js. Let's either use getFullRecordingSnapshot or getReplayRecordingContent here.
  • Let's use the normalize helper function I added to stringify the snapshot. I suggest we update the normalize function to treat the pixel/size values as they're apparently flakey. Maybe, it's enough to round them to the closes integer or we might even need to hard-redact them (like the time offset in incremental snapshots).

I've been meaning to go over the tests we added before we merged all these helpers and adjust them but didn't get to it yet. Would be great if we could adjust these two tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, thats great! I'll look into this then, thanks a lot.

WIP ?


WIP?


add .gitginore
Allow to normalize number attributes to make it a bit less flaky
@mydea mydea force-pushed the fn/replay-test-block branch from 6bf9653 to a8c3c1b Compare February 20, 2023 13:44
@mydea
Copy link
Member Author

mydea commented Feb 20, 2023

So I've updated this to use normalize, and also extended normalize with a new normalizeNumberAttributes option which rounds numbers down. This can still be flaky, but at least a bit less so!

@github-actions
Copy link
Contributor

github-actions bot commented Feb 20, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.07 KB (+0.02% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 62.3 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.69 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified) 55.31 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 20.41 KB (0%)
@sentry/browser - Webpack (minified) 66.76 KB (0%)
@sentry/react - Webpack (gzipped + minified) 20.44 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.86 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.94 KB (+0.01% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.2 KB (0%)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 42.57 KB (+0.01% 🔺)
@sentry/replay - Webpack (gzipped + minified) 36.78 KB (0%)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 60.2 KB (+0.01% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 53.8 KB (+0.01% 🔺)

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, let's go with this number normalization approach. LGTM!

@mydea mydea merged commit b04a6d4 into develop Feb 20, 2023
@mydea mydea deleted the fn/replay-test-block branch February 20, 2023 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants