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

Add clientConfig to WS test #4299

Closed
wants to merge 2 commits into from
Closed

Add clientConfig to WS test #4299

wants to merge 2 commits into from

Conversation

spacesailor24
Copy link
Contributor

We experience the issue mentioned in #3816 with our e2e_windows test suite, this aims to fix it

@spacesailor24 spacesailor24 added the 1.x 1.0 related issues label Sep 8, 2021
@render
Copy link

render bot commented Sep 8, 2021

@coveralls
Copy link

coveralls commented Sep 8, 2021

Pull Request Test Coverage Report for Build 1211808017

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 422 unchanged lines in 7 files lost coverage.
  • Overall coverage decreased (-2.6%) to 72.908%

Files with Coverage Reduction New Missed Lines %
packages/web3-core-requestmanager/src/jsonrpc.js 1 88.0%
packages/web3-core-helpers/src/formatters.js 24 80.65%
packages/web3-core-helpers/src/errors.js 31 4.41%
packages/web3-utils/src/soliditySha3.js 55 5.13%
packages/web3-utils/src/index.js 62 29.31%
packages/web3-utils/src/utils.js 85 13.04%
packages/web3-eth-accounts/src/index.js 164 25.31%
Totals Coverage Status
Change from base Build 1195730440: -2.6%
Covered Lines: 3324
Relevant Lines: 4310

💛 - Coveralls

process.env.INFURA_WSS,
{
clientConfig: {
maxReceivedFrameSize: 100000000,
Copy link
Contributor

Choose a reason for hiding this comment

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

One message can fit into one or more(if message is fragmented) data frames as https://datatracker.ietf.org/doc/html/rfc6455#section-5.6 ,

The default values are maxReceivedFrameSize 1MiB and maxReceivedMessageSize 8MiB
https://github.com/theturtle32/WebSocket-Node/blob/a2cd3065167668a9685db0d5f9c4083e8a1839f0/docs/WebSocketClient.md#client-config-options

Both maxReceivedFrameSize and maxReceivedMessageSize are made here around 95.3 MiB, which will hinder fragmentation and multiplexing https://datatracker.ietf.org/doc/html/rfc6455#section-5.4 in case of huge message.

Plus It will not make ws connection fail safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

As these are just the tests so be fine. But I agree with @jdevcs we should be using realistic values here.

Copy link
Contributor

@nazarhussain nazarhussain left a comment

Choose a reason for hiding this comment

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

Apart form the point raised by @jdevcs LGTM.

@spacesailor24
Copy link
Contributor Author

I'm going to close this until we start to see the error pop up again, so that we can test with values other than 100000000 to obtain a more reasonable value. Additionally, I'm not entirely convinced this is the solution

@spacesailor24 spacesailor24 deleted the wyatt/3816-ws-error branch September 22, 2021 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants