test(replay): Test compressed recording payloads #7082
Merged
Conversation
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
billyvg
approved these changes
Feb 8, 2023
| const envelopeBytes = request?.postDataBuffer() || ''; | ||
|
|
||
| // first, we convert the bugger to string to split and go through the uncompressed lines | ||
| const envelopeString = envelopeBytes.toString(); |
There was a problem hiding this comment.
I wonder if we'll hit some string encoding issues -- does this need to be utf8? I guess easy way to test this is to add some unicode to our tests :)
There was a problem hiding this comment.
Good point. I added it to the improvement issue. I'm gonna tackle this separately to not widen the scope of this PR.
Comment on lines
+154
to
+160
| // If we fail to parse a line, we _might_ have found a compressed payload, | ||
| // so let's check if this is actually the case. | ||
| // This is quite hacky but we can't go through `line` because the prior operations | ||
| // seem to have altered its binary content. Hence, we take the raw envelope and | ||
| // look up the place where the zlib compression header(0x78 0x9c) starts | ||
| for (let i = 0; i < envelopeBytes.length; i++) { | ||
| if (envelopeBytes[i] === 0x78 && envelopeBytes[i + 1] === 0x9c) { |
There was a problem hiding this comment.
Yikes, wish we had added some additional headers to determine the payload type. I wouldn't have expected split to change the binary content though.
There was a problem hiding this comment.
Yup, that would have been a good idea 😅 I think it's not so much the split as rather the toString call beforehand. My pragmatic take here: Let's leave it as is for now and if (when) it bites us in the back, we'll revisit.
Base automatically changed from
lms-replay-playwright-recording-tests
to
develop
February 8, 2023 07:53
Lms24
force-pushed
the
lms-replay-pw-compression
branch
from
6fd1984 to
2f2ab9c
Compare
size-limit report 📦
|
This was referenced Feb 8, 2023
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.
This PR adds the Playwright integration test utilities to test Replay's compression. We now have a dedicated parser that attempts to decompress a payload, if it encounters a zlib-compressed replay recording payload. Took me quite a while to figure out how this works and admittedly, the solution I came up with is a bit hacky (see code).
Additionally, this PR adds a simple test to check that an initial full compressed snapshot has the correct content.
Note: Not sure why the
worker.jsfile changed. Before this PR we already used[email protected]for the worker and the only thing I did was addingpakoto theintegration-testspackage. Regardless, I think this should be finedepends on #7052
ref #7044