-
Notifications
You must be signed in to change notification settings - Fork 4
Conversation
/cc @flotwig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @cardil thanks for opening this. Sorry that this flew under the radar. We're working on improving our issue management processes on the team so this happens less often.
Can you please go ahead and create a PR to the cypress-io/cypress
repo that points http-proxy
to cardil/node-http-proxy#eec6ed5f0d65d34faf8bf90a1727764ebc334e81
? That way we can run this change with the existing proxy tests.
packages/server/test/integration/websockets_spec.js
is an excellent place to add a test for this. cypress-io/cypress#6945 should have had tests but didn't, because I wasn't sure what steps could reproduce this. These tests can be run via yarn test-integration websockets
inside of packages/server
If the PR to cypress-io/cypress
is passing, then this LGTM.
I'm not sure either what setup would be necessary to reproduce conditions in which the error occurs. What I can assure that this change helps. After applying openshift-knative/serverless-operator#1099, we've stopped seeing the |
@cardil having a PR in
I'll try to add tests to your PR there if it doesn't break any of the CI in that repo, but if it ends up being a rabbithole, we can get this merged without it. |
Thanks @cardil for this fix! I can also confirm after manually applying this change, I have stopped seeing |
Hi @nanek . Need your help in setting this up, |
This is still an issue so I went ahead and made a PR to the main Cypress repository to validate this doesn't break existing proxy workflows here: cypress-io/cypress#29453 |
@JamesNimlos Thanks, we'll take a new look at this again. |
closing in favor of cypress-io/cypress#29499 |
Partially fixes cypress-io/cypress#17288
This patch ensures that we write to a socket that is, in fact, open. This prevents exceptions from being thrown, like evidenced in cypress-io/cypress#17288.
After merge, this needs to be bundled into cypress to fix cypress-io/cypress#17288 fully.