Skip to content
This repository was archived by the owner on Aug 2, 2021. It is now read-only.

Add tests for buffered http get#725

Merged
zelig merged 2 commits into
swarm-network-rewritefrom
bufferedget-test
Jun 19, 2018
Merged

Add tests for buffered http get#725
zelig merged 2 commits into
swarm-network-rewritefrom
bufferedget-test

Conversation

@janos
Copy link
Copy Markdown
Member

@janos janos commented Jun 18, 2018

No description provided.

@janos janos requested review from acud, gbalint and zelig June 18, 2018 11:24
Copy link
Copy Markdown
Contributor

@acud acud left a comment

Choose a reason for hiding this comment

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

@janos this generally LGTM however I have the following concerns:

  • i'm not sure that this is exhaustive with the original ioCopyBufferSize - 32kb is not that much, taking into account os caching and os native kernel reader buffer sizes. my assumption they will vary greatly in different systems under different hardware conditions. the variance in uploaded file size is also not big enough IMHO. to get a proper regression I'd aim for bigger numbers and have the test run for a longer time.
  • flakyness alert - you don't think this will flake quite often? are the slope values travis adjusted? i'm usually not in favour of having hardcoded relative result values running on a CI - this asks for trouble. boolean assertions are better

@janos
Copy link
Copy Markdown
Member Author

janos commented Jun 18, 2018

@justelad Thanks for the review.

  1. The buffer that is in question is this one here https://github.com/golang/go/blob/master/src/io/io.go#L391 which is fixed and we are addressing with buffered read only this issue, regardless of other caching on lower levels.

Would you like to add a -longrunning variation of tests for bigger files? This sizes with slower get's are in order of magnitude that are acceptable for standard tests on Travis.

  1. I have adjusted the slope values to be more tolerant for Travis in the latest commit. I could not think any boolean assertion that would satisfy the performance change detection. Do you have some suggestions beside linear regression?

@zelig zelig merged commit 1039a4b into swarm-network-rewrite Jun 19, 2018
@frncmx frncmx deleted the bufferedget-test branch January 22, 2019 09:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants