Skip to content
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

fix: network capture tests should fail if we exhaust the body #1395

Merged
merged 9 commits into from
Aug 30, 2024

Conversation

pauldambra
Copy link
Member

@pauldambra pauldambra commented Aug 30, 2024

part of responding to PostHog/posthog#24471 due to the unexpected consequences of #1351

it is very hard to test for all unexpected consequences of changes to code 🙈

but we do at least now know for sure that that we cannot rely on request/response objects being readable and that we should not read them and then pass them on

Screenshot 2024-08-30 at 10 03 11

Here's the test failing in CI and showing we would have avoided all of the painful impact for folk

  1 failing
  1) Session recording
       network capture - when fetch wrapper is not badly behaved
         it sends network payloads:
     TypeError: The following error originated from your test code, not from Cypress. It was caused by an unhandled promise rejection.
  > Failed to execute 'fetch' on 'Window': Cannot construct a Request with a Request object that has already been used.

Copy link

vercel bot commented Aug 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Aug 30, 2024 11:22am

@pauldambra
Copy link
Member Author

pauldambra commented Aug 30, 2024

@daibhin @neilkakkar just a heads up on this in case you spot anything, but don't worry too much, it's very much a spike at the moment

Copy link

github-actions bot commented Aug 30, 2024

Size Change: +213 B (+0.02%)

Total Size: 1.17 MB

Filename Size Change
dist/array.full.js 335 kB +71 B (+0.02%)
dist/recorder-v2.js 110 kB +71 B (+0.06%)
dist/recorder.js 110 kB +71 B (+0.06%)
ℹ️ View Unchanged
Filename Size
dist/array.js 156 kB
dist/exception-autocapture.js 10.4 kB
dist/main.js 157 kB
dist/module.js 156 kB
dist/surveys-preview.js 59.8 kB
dist/surveys.js 66 kB
dist/tracing-headers.js 8.26 kB
dist/web-vitals.js 5.79 kB

compressed-size-action

.finally(() => clearTimeout(timeout))
} catch (e) {
clearTimeout(timeout)
resolve('[SessionReplay] Failed to read body')
Copy link
Member Author

@pauldambra pauldambra Aug 30, 2024

Choose a reason for hiding this comment

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

this is a genuine fix for an actual bug, not just a test addition

errors in .text() are caught in the then and rejected

but r.clone() can throw and we wouldn't catch that

so instead we should catch that error, and resolve with something meaningful, that means our captured network request would have this string as its body

and everything else should carry on

Copy link
Member Author

Choose a reason for hiding this comment

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

it's not the only way to make this fail though.. it isn't the bug

@pauldambra pauldambra changed the title fix: network capture should not exhaust the body fix: network capture tests should fail if we exhaust the body Aug 30, 2024
@pauldambra pauldambra marked this pull request as ready for review August 30, 2024 11:07
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.

2 participants