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

Improve too large chunk size test #529

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

SkyTrix
Copy link
Contributor

@SkyTrix SkyTrix commented Apr 2, 2024

This is a followup of #528

The current test goes over the configured max chunk size when parsing the last hex char. The spirit of the code is to continue parsing the size even if we've already gone over the max chunk size. To actually test this we need to make sure another char follows after we've gone over the max chunk size.

The current test goes over the configured max chunk size when parsing
the last hex char. The spirit of the code is to continue parsing the
size even if we've already gone over the max chunk size. To actually
test this we need to make sure another char follows after we've gone
over the max chunk size.
Copy link
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

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

Lgtm

@pjfanning
Copy link
Contributor

@SkyTrix how long does this test take to run? A couple of seconds is ok but we ideally would not have long running tests.

@SkyTrix
Copy link
Contributor Author

SkyTrix commented Apr 2, 2024

@SkyTrix how long does this test take to run? A couple of seconds is ok but we ideally would not have long running tests.

This test, like most others in this file, is pretty simple and runs in 5 milliseconds on my machine. This just reads and compares a hex number, there is no 20MB chunk anywhere to worry about.

@He-Pin
Copy link
Member

He-Pin commented Apr 2, 2024

Maybe the value can be adjusted with the configuration value read from the default configuration, better than hard cording.

@pjfanning pjfanning self-requested a review April 2, 2024 19:00
Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm - if the test still runs quickly, it is ok to upgrade

@pjfanning pjfanning merged commit ced1ff4 into apache:main Apr 2, 2024
9 of 10 checks passed
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.

3 participants