test(replay): Add integration tests for input masking on change by billyvg · Pull Request #7260 · getsentry/sentry-javascript

billyvg · 2023-02-22T22:49:27Z

This adds integration tests for input masking specifically when maskAllInputs = false.

Requires getsentry/rrweb#61 and getsentry/rrweb#60
Closes#7257

github-actions · 2023-02-22T22:54:53Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	20.16 KB (+0.24% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)	62.64 KB (+0.21% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.8 KB (+0.26% 🔺)
@sentry/browser - ES6 CDN Bundle (minified)	55.64 KB (+0.27% 🔺)
@sentry/browser - Webpack (gzipped + minified)	20.55 KB (+0.24% 🔺)
@sentry/browser - Webpack (minified)	67.14 KB (+0.23% 🔺)
@sentry/react - Webpack (gzipped + minified)	20.58 KB (+0.24% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified)	48.31 KB (+0.08% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	27.3 KB (+0.17% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	25.57 KB (+0.21% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified)	43.15 KB (+0.08% 🔺)
@sentry/replay - Webpack (gzipped + minified)	37.16 KB (0%)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	60.96 KB (+0.09% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	54.28 KB (+0.09% 🔺)

billyvg · 2023-02-22T22:55:00Z

ref: #7044

github-actions · 2023-02-23T00:20:17Z

Replay SDK metrics 🚀

		Plain	+Sentry			+Replay
	Revision	Value	Value	Diff	Ratio	Value	Diff	Ratio
LCP	This PR `e24127c`	77.33 ms	98.06 ms	+20.73 ms	+26.80 %	123.39 ms	+46.06 ms	+59.56 %
LCP	Previous `e60cd02`	95.19 ms	116.88 ms	+21.69 ms	+22.79 %	151.44 ms	+56.25 ms	+59.09 %
CLS	This PR `e24127c`	0.06 ms	0.06 ms	-0.00 ms	-0.70 %	0.06 ms	-0.00 ms	-0.38 %
CLS	Previous `e60cd02`	0.06 ms	0.06 ms	-0.00 ms	-0.45 %	0.06 ms	-0.00 ms	-0.37 %
CPU	This PR `e24127c`	13.73 %	12.73 %	-1.00 pp	-7.29 %	18.22 %	+4.49 pp	+32.71 %
CPU	Previous `e60cd02`	18.74 %	17.90 %	-0.85 pp	-4.52 %	25.06 %	+6.32 pp	+33.70 %
JS heap avg	This PR `e24127c`	1.94 MB	1.99 MB	+44.16 kB	+2.27 %	2.87 MB	+930.41 kB	+47.89 %
JS heap avg	Previous `e60cd02`	1.94 MB	2 MB	+62.53 kB	+3.22 %	2.87 MB	+927.44 kB	+47.82 %
JS heap max	This PR `e24127c`	2.31 MB	2.56 MB	+246.92 kB	+10.68 %	3.36 MB	+1.04 MB	+45.14 %
JS heap max	Previous `e60cd02`	2.3 MB	2.57 MB	+265.21 kB	+11.52 %	3.37 MB	+1.06 MB	+46.20 %
netTx	This PR `e24127c`	0 B	0 B	0 B	n/a	2.22 kB	+2.22 kB	n/a
netTx	Previous `e60cd02`	0 B	0 B	0 B	n/a	2.21 kB	+2.21 kB	n/a
netRx	This PR `e24127c`	0 B	0 B	0 B	n/a	41 B	+41 B	n/a
netRx	Previous `e60cd02`	0 B	0 B	0 B	n/a	41 B	+41 B	n/a
netCount	This PR `e24127c`	0	0	0	n/a	1	+1	n/a
netCount	Previous `e60cd02`	0	0	0	n/a	1	+1	n/a
netTime	This PR `e24127c`	0.00 ms	0.00 ms	0.00 ms	n/a	89.44 ms	+89.44 ms	n/a
netTime	Previous `e60cd02`	0.00 ms	0.00 ms	0.00 ms	n/a	117.55 ms	+117.55 ms	n/a

Previous results on branch: `develop`

Revision	LCP	CLS	CPU	JS heap avg	JS heap max	netTx	netRx	netCount	netTime
`e60cd02`	+56.25 ms	-0.00 ms	+6.32 pp	+927.44 kB	+1.06 MB	+2.21 kB	+41 B	+1	+117.55 ms
`e25c067`	+48.34 ms	+0.00 ms	+5.59 pp	+926.37 kB	+1.05 MB	+2.22 kB	+41 B	+1	+65.23 ms
`b1b249b`	+43.88 ms	+0.00 ms	+4.80 pp	+937.99 kB	+1.05 MB	+2.22 kB	+41 B	+1	+111.56 ms
`12e34d4`	+28.57 ms	+0.00 ms	+5.77 pp	+930.12 kB	+1.04 MB	+2.26 kB	+41 B	+1	+109.67 ms
`c46c56c`	+65.45 ms	-0.00 ms	+5.38 pp	+930.26 kB	+1.07 MB	+2.21 kB	+41 B	+1	+91.29 ms
`7f4c4ec`	+56.64 ms	-0.00 ms	+5.57 pp	+927.42 kB	+1.06 MB	+2.21 kB	+41 B	+1	+110.83 ms
`00d2360`	+55.18 ms	+0.00 ms	+2.23 pp	+934.14 kB	+1.05 MB	+2.22 kB	+41 B	+1	+71.65 ms

*) pp - percentage points - an absolute difference between two percentages.
Last updated: Thu, 23 Feb 2023 08:55:46 GMT

Lms24 · 2023-02-23T08:35:09Z

+ await page.locator('#input').type(text)
+ const snapshots = getIncrementalRecordingSnapshots(await reqPromise1).filter(isInputMutation);
+ const lastSnapshot = snapshots[snapshots.length-1];
+ expect(lastSnapshot.text).toBe(text);


Can we be sure that this snapshot always comes last? If yes, no problem! Just asking because I observed quite some flakiness with incremental snapshots across different browsers.

Yeah I observed a bunch of flake too from mouse movements I think -- having the isInputMutation filter helps with that and those are sorted (otherwise playing back input changes would not work as expected).

billyvg · 2023-03-02T15:51:59Z

+ 'should mask input initial value and its changes',
+ async ({ browserName, forceFlushReplay, getLocalTestPath, page }) => {
+ // TODO(replay): This is flakey on firefox where we do not always get the latest mutation.
+ if (shouldSkipReplayTest() || browserName === 'firefox') {


How do yall feel about this lol, I was not able to get this to work reliably on firefox @Lms24 @mydea

I think @Lms24 did the same for some other test, IMHO it is fine.

Yup, let's skip this on FF

mydea · 2023-03-03T09:57:53Z

@billyvg we just discussed in the team that in order to avoid flaky tests, whenever we introduce new integration tests that have the potential to be flaky we want to make sure to run them 100x on CI to figure out if they are flaky before we merge.

So basically, could you wrap the new test(s) like here: 53dd25a, and only when this passes, remove it again before we merge?

billyvg · 2023-03-06T17:15:41Z

@mydea Looks like webkit also flakes at ~<5% of the time -- should we skip webkit as well?

billyvg · 2023-03-06T22:25:40Z

I was just curious so I reduced the text to type out in the test to 4 characters (down from 14), webkit failed twice total -- and luckily for me these tests ended up flaking D:

2x - [firefox] › suites/tracing/browsertracing/backgroundtab-pageload/test.ts:7:11 › should finish pageload transaction when the page goes background
1x - [chromium] › suites/stacktraces/protocol_containing_fn_identifiers/test.ts:53:11 › should not add any part of the function identifier to beginning of filename

I still think we should skip webkit, but I'll keep the reduced text

mydea · 2023-03-07T08:12:18Z

👍 This is fine, IMHO better to skip on webkit than to flake every now and then. Let's

mydea · 2023-03-07T09:23:14Z

FYI I will merge this in as I will open a PR moving some package folders around a bit, so merging this in first will avoid merge conflicts later.

This adds integration tests for input masking specifically when `maskAllInputs = false`. remove unused snapshots skip firefox for these tests due to flakeyness TEMP: run 100x shorter test text Revert "TEMP: run 100x" This reverts commit 08967e2. skip webkit

run 100 times fix flaky? unrun 100

billyvg · 2023-03-08T20:55:12Z

Flaked: [chromium] › suites/stacktraces/protocol_fn_identifiers/test.ts:7:11 › should parse function identifiers that are protocol names correctly

mydea · 2023-03-09T07:29:18Z

Flaked: [chromium] › suites/stacktraces/protocol_fn_identifiers/test.ts:7:11 › should parse function identifiers that are protocol names correctly

Jup, but that's unrelated to this PR at least, so I can finally merge this xD

billyvg mentioned this pull request Feb 22, 2023
Session Replay Privacy do not work as expected, sentry-mask show full text after changes #7257
Closed
3 tasks

billyvg requested review from Lms24 and mydea February 22, 2023 23:00

Lms24 approved these changes Feb 23, 2023
View reviewed changes

billyvg force-pushed the test-replay-add-integration-test-input-masking branch 2 times, most recently from ccd4bb9 to e3edf79Compare February 28, 2023 21:52

mydea approved these changes Mar 1, 2023
View reviewed changes

billyvg force-pushed the test-replay-add-integration-test-input-masking branch 2 times, most recently from a5bce54 to 3683461Compare March 1, 2023 20:40

billyvg commented Mar 2, 2023
View reviewed changes

billyvg marked this pull request as ready for review March 2, 2023 19:07

billyvg force-pushed the test-replay-add-integration-test-input-masking branch from b618a74 to 08967e2Compare March 3, 2023 22:44

mydea force-pushed the test-replay-add-integration-test-input-masking branch 7 times, most recently from 104e54f to 8521f87Compare March 8, 2023 16:31

mydea added 3 commits March 8, 2023 18:38

ref: Use fill instead of type
d6d3371

refd: Improve breadcrumb handling
e4da1f8

fix flaky test?
ea80817
run 100 times fix flaky? unrun 100

mydea force-pushed the test-replay-add-integration-test-input-masking branch from 8521f87 to ea80817Compare March 8, 2023 17:38

mydea merged commit c192c9a into developMar 9, 2023

mydea deleted the test-replay-add-integration-test-input-masking branch March 9, 2023 07:30

Uh oh!

Uh oh!

Conversation

billyvg commented Feb 22, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actionsBot commented Feb 22, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

billyvg commented Feb 22, 2023

Uh oh!

github-actionsBot commented Feb 23, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Replay SDK metrics 🚀

Previous results on branch: develop

Uh oh!

Lms24Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

billyvgFeb 23, 2023

Choose a reason for hiding this comment

Uh oh!

billyvgMar 2, 2023

Choose a reason for hiding this comment

Uh oh!

mydeaMar 3, 2023

Choose a reason for hiding this comment

Uh oh!

Lms24Mar 3, 2023

Choose a reason for hiding this comment

Uh oh!

mydea commented Mar 3, 2023

Uh oh!

billyvg commented Mar 6, 2023

Uh oh!

billyvg commented Mar 6, 2023

Uh oh!

mydea commented Mar 7, 2023

Uh oh!

mydea commented Mar 7, 2023

Uh oh!

billyvg commented Mar 8, 2023

Uh oh!

mydea commented Mar 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

billyvg commented Feb 22, 2023•
edited
Loading

github-actionsBot commented Feb 22, 2023•
edited
Loading

github-actionsBot commented Feb 23, 2023•
edited
Loading

Previous results on branch: `develop`