tools: add tap output to cpplint #3448
Conversation
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/python2.4 | |||
| #!/usr/bin/env python | |||
There was a problem hiding this comment.
Not that I care much, but this wouldn't work on Arch Linux where python is python3.
There was a problem hiding this comment.
I don't think python2.4 would work much better :) Just pulled the same convention as what we have in configure and tools/test.py -- would personally have gone with python2 but +1 for consistency.
There was a problem hiding this comment.
Oh, right. I have a python2 and a python2.7. python2 would also have been my choice, but whatever is consistent is fine.
|
LGTM with a nit and as long as it doesn't make moving to other tools in the future harder. |
|
My magic ball speaks of clang-format, but we'll see when that happens. Will fix the quotes (will I ever learn). |
|
As for moving into other tools forward, having TAP as a required output format (well, I think we can use junit from jenkins as well) seems reasonable. cc @nodejs/build. |
|
So this prints one It's missing the TAP plan. LGTM |
| sys.stderr.write('Category \'%s\' errors found: %d\n' % | ||
| (category, count)) | ||
| sys.stderr.write('Total errors found: %d\n' % self.error_count) | ||
| if not _cpplint_state.output_format == 'tap': |
There was a problem hiding this comment.
Any specific reason to use this instead of if _cpplint_state.output_format != 'tap':?
That's not very useful. It makes it very hard to differentiate between a crash and a successful run if an empty file indicates success. This should be easy enough to address with the same code that will be added to fix the lack of plan. |
|
What's the status of this PR? Blocked on @rmg's comment? |
|
Not really. I think we should land and fix as we move forward. This patch will give us visibility (here are the fails) where there currently is pass or fail. |
|
Let's have a CI run and check the result? |
|
@thefourtheye won't be invoked through ci just now. |
| logger.setLevel(logging.INFO) | ||
|
|
||
| if _cpplint_state.output_format == 'tap': | ||
| logger.info('TAP Version 13') |
There was a problem hiding this comment.
As per the spec, it should have been version, no?
There was a problem hiding this comment.
I think all TAP is case insensitive, but yeah – the spec does use lowercase. I copied above from eslint output. Will update.
This actually is kind of problematic for the TAP result parser I guess. We don't print the plan and we don't print the current test number as well. So, how will the parser keep track of the current test which failed? |
|
@thefourtheye it won't keep track of tests between runs, but it will report test failures with verbose info each run. lint failures are so rare; my take was that we'd rather get more output than no. |
|
Fair enough. Considering this affects only the cpp linter, LGTM. Do you have plans to extend this to test runner? |
|
@thefourtheye yeah. I've already got a branch i've been running tests on locally (and on my gh) so we can expose this as part of the test runner. |
|
@jbergstroem Awesome! |
jbergstroem
force-pushed
the
feature/cpplint-tap
branch
2 times, most recently
from
f041203 to
6c3d194
Compare
Implement a crude TAP13 writer for cpplint. Does its job and not much else. Only supports writing TAP output to file, not vs7 or emacs formats. PR-URL: nodejs#3448 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
jbergstroem
force-pushed
the
feature/cpplint-tap
branch
from
6c3d194 to
7b45163
Compare
Implement a crude TAP13 writer for cpplint. Does its job and not much else. Only supports writing TAP output to file, not vs7 or emacs formats. PR-URL: #3448 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Implement a crude TAP13 writer for cpplint. Does its job and not much else. Only supports writing TAP output to file, not vs7 or emacs formats. PR-URL: #3448 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Implement a crude TAP13 writer for cpplint. Does its job and not much else. Only supports writing TAP output to file, not vs7 or emacs formats. PR-URL: #3448 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Implement a crude TAP13 writer for cpplint. Does its job and not much else. Only supports writing TAP output to file, not vs7 or emacs formats.