Put os.Stdin size to NewReaderSize instead NewReader #441
Conversation
Codecov Report
@@ Coverage Diff @@
## master #441 +/- ##
==========================================
+ Coverage 30.89% 31.16% +0.27%
==========================================
Files 26 26
Lines 3208 3215 +7
==========================================
+ Hits 991 1002 +11
+ Misses 2122 2117 -5
- Partials 95 96 +1
Continue to review full report at Codecov.
|
|
Hi @IgorHalfeld - thanks for the PR 🙏 we appreciate it. Can you explain what the issue is here please? The issue that you have linked refers to a problem splitting test results, and this patch changes the function that reads contexts. Did you link to the wrong issue? |
|
@marcomorain I updated the PR description |
|
I fixed code coverage 🚀 IMHO I think you shouldn't use Ginkgo for testing, the Go standard test tool with Testify is the best choice for a number of factors, one of which is performance. I used Ginkgo for 1 year and a half at work, and when we got to a large codebase we started having problems with test execution time. |
|
any plans to get this in? it is breaking a lot of our circleci tests jobs |
|
any progress on this? |
|
Hope a merge soon. We have a lot of tests, fails a lot because of this |
|
Can we have this one merged? |
|
I'll get someone to review this today 👍 |
|
what about today? |
There was a problem hiding this comment.
Thank you @IgorHalfeld, and apologies for the delay! I just had a few questions regarding this PR.
| stat, _ := os.Stdin.Stat() | ||
|
|
||
| buffSize, err := os.Stdin.Stat() |
There was a problem hiding this comment.
To avoid the redundancy, would it be possible to remove this Stat() call and instead use the one on line 174?
| buffSize, err := os.Stdin.Stat() | |
| stat, err := os.Stdin.Stat() | |
| if err != nil { | |
| return "", err | |
| } | |
| reader := bufio.NewReaderSize(os.Stdin, int(stat.Size())) | |
| // ... |
| bytes, err := ioutil.ReadAll(os.Stdin) | ||
| bytes := make([]byte, buffSize.Size()) | ||
| _, err := io.ReadFull(reader, bytes) |
There was a problem hiding this comment.
Did you happen to experience errors other than failed to read input: bufio.Scanner: token too long from this code path?
I believe that ioutil.ReadAll will read the entire contents of the provided io.Reader without any limitations. It seems that '512' refers to the initial buffer size, which will dynamically grow until EOF/OOM occurs, so I'm not certain changing this to io.ReadFull will address that error specifically.
|
Merge conflicts need to be resolved. Context create was recently updated to accept OrgId |
|
it seems we are hitting the same issue, and we have only? 653 tests to run, see here. Are there any plans to fix this? Or may be a workaround? |
|
@wind57 @IgorHalfeld This PR is conflicting with master. The quickest solution is for a fresh PR to be made by an external contributor and we can look into it from there. |
solves #439
In Golang
NewReader, the default is 4069 for the Buffer size.What I did was use
NewReaderSizeto put the Buffer size as the size of os.Stdin.So it doesn't give that error of:
Error: failed to read input: bufio.Scanner: token too longI changed
ioutil.ReadAll(default size is 512) to.io.ReadFulltoo