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 test output handling #1513

Merged
merged 1 commit into from
Nov 21, 2017
Merged

Conversation

bhcleek
Copy link
Collaborator

@bhcleek bhcleek commented Oct 15, 2017

  • Use stricter regular expression to correctly identify potential file
    references in test output. When go test outputs a test failure file
    location, the file is preceded with at least one tab. If there is
    leading space on the line, then it's a subtest that failed. Use that
    to more accurately disregard lines that will not refer to files.

  • Preserve multi-line test failure output by similarly checking for at
    least one tab ahead of the output.

  • Preserve output that seemingly should refer to a file, but for
    whatever does not, but don't make it a file reference for the purposes
    of quickfix and location lists.

  • Preserve all lines of stack traces.

  • Correctly recognize file locations in stack traces.

Fixes #1179 and at least partially addresses #1493.

let tokens = matchlist(line, '^\t\+\(.\{-}\.go\):\(\d\+\) \(+0x.*\)')
else
let tokens = matchlist(line, '^ *\t\+\(.\{-}\.go\):\(\d\+\):\s*\(.*\)')
endif
Copy link
Owner

Choose a reason for hiding this comment

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

Could you add as a comment above the let tokens ... lines what exactly it parses? Such as : foo.go:1:15 msg It's would make reading the regexp easier.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@bhcleek
Copy link
Collaborator Author

bhcleek commented Oct 18, 2017

The latest commit, 9e56464, provides the comment you're looking for. I just did git commit --fixup and will git rebase -i --autosquash before merging after approval.

PTAL

@arp242
Copy link
Contributor

arp242 commented Oct 18, 2017

I think this would be an excellent patch to add some tests for, so we can be sure that all scenarios work :-)

@bhcleek
Copy link
Collaborator Author

bhcleek commented Oct 18, 2017

I agree, but I've yet to dive in to see how to write tests for VimL. Do you want to wait to merge this until those tests are written?

@arp242
Copy link
Contributor

arp242 commented Oct 18, 2017

Don't need to; just would be nice :-)

Writing tests is actually easier than I thought it would be, I have some work on making it easier (and documenting how to do it) in this branch (which depends on #1476).

I'd be happy to add tests for this (either before or after it's merged).

@bhcleek
Copy link
Collaborator Author

bhcleek commented Oct 18, 2017

Yeah, I was kind of hoping to review that PR before I write tests for this, but I agree that we should add test coverage for this.

#1476 is big enough that I plan to use reviewing it as a good excuse write tests for this while reviewing it in order to understand exactly what #1476 does and what the experience it brings to testing will be like.

@bhcleek
Copy link
Collaborator Author

bhcleek commented Oct 20, 2017

@fatih can you review f747c96 to make sure I addressed your concerns about the comments for the regexes sufficiently?

@bhcleek bhcleek force-pushed the fix-test-error-processing branch from 9e56464 to f747c96 Compare October 30, 2017 05:42
@bhcleek
Copy link
Collaborator Author

bhcleek commented Oct 30, 2017

Here's a tarball with tests that produce:

  • panic
  • multi-line test failure message
  • sub-test failures
  • test log messages (e.g. "testing.T".Log)

vim-go-test-parsing.tar.gz

@arp242 arp242 force-pushed the fix-test-error-processing branch from f747c96 to 260f7c2 Compare November 4, 2017 09:59
@bhcleek
Copy link
Collaborator Author

bhcleek commented Nov 19, 2017

@fatih does the comment I added address your concerns?

@Carpetsmoker you had mentioned that you may add tests for this; have you made any progress?

I'd really like to get this merged, as I run into the issues it addresses daily.

@arp242
Copy link
Contributor

arp242 commented Nov 21, 2017

Yes, I wanted to do that but got side-tracked with some other stuff and never got 'round to it, and it's not likely that I will in the coming week either.

If it's fixing a real problem for you then feel free to merge; don't need to add tests, just would be nice :-)

* Use stricter regular expression to correctly identify potential file
  references in test output. When `go test` outputs a test failure file
  location, the file is preceded with at least one tab. If there is
  leading space on the line, then it's a subtest that failed. Use that
  to more accurately disregard lines that will not refer to files.

* Preserve multi-line test failure output by similarly checking for at
  least one tab ahead of the output.

* Preserve output that seemingly *should* refer to a file, but for
  whatever does not, but don't make it a file reference for the purposes
  of quickfix and location lists.

* Preserve all lines of stack traces.

* Correctly recognize file locations in stack traces.
@bhcleek bhcleek force-pushed the fix-test-error-processing branch from 260f7c2 to 57ac640 Compare November 21, 2017 01:59
@bhcleek bhcleek merged commit ca8ef8a into fatih:master Nov 21, 2017
@bhcleek bhcleek deleted the fix-test-error-processing branch November 21, 2017 02:08
@bhcleek
Copy link
Collaborator Author

bhcleek commented Nov 21, 2017

SGTM

I'll work on adding tests separately.

@bhcleek bhcleek mentioned this pull request Nov 21, 2017
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.

Printing unparsed test errors in Vim 8 silently fails
3 participants