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

fix quickfix list when test duration exceeds timeout #1633

Merged
merged 1 commit into from
Jan 3, 2018

Conversation

bhcleek
Copy link
Collaborator

@bhcleek bhcleek commented Dec 31, 2017

Do not treat a panic caused by a test timeout as an error that occurred
within a test's callstack. When go test panics due to a test timeout,
the running goroutine's stacktrace only contains entries for the
standard library, and there is one goroutine whose stacktrace contains
entries for the standard library and the test binary. None of the other
goroutines should be considered to contain an error, either, because the
panic was not due to any code executing within those goroutines' stacks.
Therefore, treat panics caused by a test timeout as an error without a
file location.

@codecov-io
Copy link

Codecov Report

Merging #1633 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1633      +/-   ##
==========================================
+ Coverage   19.17%   19.21%   +0.03%     
==========================================
  Files          53       53              
  Lines        4156     4158       +2     
==========================================
+ Hits          797      799       +2     
  Misses       3359     3359
Flag Coverage Δ
#nvim 14.52% <0%> (-0.01%) ⬇️
#vim74 17.6% <100%> (+0.03%) ⬆️
#vim80 18.37% <100%> (+0.03%) ⬆️
Impacted Files Coverage Δ
autoload/go/test.vim 69.09% <100%> (+0.37%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb99031...4895313. Read the comment docs.

\ {'lnum': 0, 'bufnr': 0, 'col': 0, 'valid': 1, 'vcol': 0, 'nr': -1, 'type': '', 'pattern': '', 'text': 'panic: test timed out after 2s'}
\ ]

let g:go_test_timeout="2s"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to use a shorter timeout here, like 100ms? Or at the very least 1s? 2 seconds isn't that long, but if you're modifying something then it's just too long for a "smooth" modify/test workflow, and these small timeouts tend to add up once the testing coverage increases.

If it's not easily possible then don't bother. It's a very small thing. Looks good otherwise 👍.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's possible. I just chose two seconds because it's reasonably short, and I was pretty sure everything that needs to start would actually be started in that time. I'll try dialing it back and will merge with the reduced timeout or this if it's not possible to reduce it reliably...

Do not treat a panic caused by a test timeout as an error that occurred
within a test's callstack. When go test panics due to a test timeout,
the running goroutine's stacktrace only contains entries for the
standard library, and there is one goroutine whose stacktrace contains
entries for the standard library and the test binary. None of the other
goroutines should be considered to contain an error, either, because the
panic was not due to any code executing within those goroutines' stacks.
Therefore, treat panics caused by a test timeout as an error without a
file location.
@bhcleek bhcleek merged commit 8eaa1a4 into fatih:master Jan 3, 2018
@bhcleek bhcleek deleted the test-timeout-errors branch January 3, 2018 01:25
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