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: do not treat utf8 requests as binary #15946

Merged
merged 12 commits into from
Apr 22, 2021
Merged

fix: do not treat utf8 requests as binary #15946

merged 12 commits into from
Apr 22, 2021

Conversation

bahmutov
Copy link
Contributor

@bahmutov bahmutov commented Apr 12, 2021

User facing changelog

We were incorrectly detecting "normal" utf8 request body as binary encoding, which caused the test runner crash. The detection was caused by multi-byte Unicode characters in some languages.

Additional details

  • this PR simply prevents the wrong detection by looking at the content-type request header, with fallback to the existing method.

PS: there is an open issue for the existing module we use to detect the encoding bevry/istextorbinary#13 which exactly matches our issue.

How has the user experience changed?

No more crashing in the provided reproducible example

PR Tasks

  • Have tests been added/updated?
  • Has the original issue or this PR been tagged with a release in ZenHub?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Apr 12, 2021

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Apr 12, 2021



Test summary

9496 0 111 3Flakiness 1


Run details

Project cypress
Status Passed
Commit ebc2cf4
Started Apr 22, 2021 3:55 PM
Ended Apr 22, 2021 4:07 PM
Duration 12:42 💡
OS Linux Debian - 10.8
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

cypress/integration/retries.ui.spec.js Flakiness
1 runner/cypress retries.ui.spec > opens attempt on each attempt failure for the screenshot, and closes after test passes

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@bahmutov bahmutov requested a review from flotwig April 12, 2021 20:12
@bahmutov
Copy link
Contributor Author

Hmm, the integration test does not fail even for previous settings, really want to see a failing test first

@bahmutov
Copy link
Contributor Author

The e2e test fails without the fix, passes with the fix

@bahmutov bahmutov marked this pull request as ready for review April 13, 2021 01:28
@bahmutov bahmutov requested a review from a team as a code owner April 13, 2021 01:28
@github-actions
Copy link
Contributor

Internal Jira issue: TR-749

Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

To check my understanding, this does not affect #15898 and #15851 right? It fixes #15901 by no longer misinterpreting the request as binary content, but does not affect the other 2 issues causing the crash with true binary content?

packages/net-stubbing/lib/server/util.ts Outdated Show resolved Hide resolved
packages/server/test/e2e/7_issue_15901_spec.js Outdated Show resolved Hide resolved
packages/net-stubbing/lib/server/middleware/request.ts Outdated Show resolved Hide resolved
@bahmutov
Copy link
Contributor Author

I don't know about #15851 or #15898 because there is no reproducible example yet

@gearsdigital
Copy link

Is there as known workarround?

@matheuspiment
Copy link

Is there as known workarround?

#15901 (comment)

tgriesser added a commit that referenced this pull request Apr 26, 2021
* develop: (49 commits)
  fix: `cy.type()` should not change the value attr of button-like inputs. (#16154)
  fix: properly detect `cy.intercept(url, routeMatcher, handler)` overload (#16167)
  fix: consider multiple routes when looking for aliases (#16180)
  fix: pass contextIsolation: true (#16165)
  chore: fix failing "should handle aborted requests" test (#16170)
  feat(issue-3741): added keyboard support for folder (#15648)
  fix(webpack-dev-server): remove hard dependency on html-webpack-plugin v4  (#16108)
  chore(deps): downgrade electron to v12.0.0-beta.14 (#16113)
  fix: accept absolute paths in vite dev server (#16148)
  fix: cy.then shows wrong type when collection of HTMLElement's is provided (#15869)
  fix: do not treat utf8 requests as binary (#15946)
  chore: fix types
  docs: fix broken links for @cypress/react example recipes (#15674)
  update circle yml
  ignore undefined beforeEach
  fix: make vite-dev-server work on windows (#16103)
  chore: add triple slash reference
  chore: remove conflicting types
  chore: rebuild yarn lock
  resolve conflicts in master(fe0b63c) and develop
  ...
Comment on lines +1321 to +1322
cy.intercept('POST', 'http://localhost:5000/api/sample')
cy.visit('/fixtures/utf8-post.html')
Copy link
Member

Choose a reason for hiding this comment

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

Since the fetch(...) is async you need to cy.wait(...) for the intercept to finish otherwise there's a race condition where the test could finish before the fetch() does and lead to a false negative.

@flotwig flotwig deleted the crash-post branch January 24, 2022 18:20
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.

[7.0.1]Bug: cy.intercept POST terminates cypress process
6 participants