-
Notifications
You must be signed in to change notification settings - Fork 30k
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
test.py tap13 output #9262
test.py tap13 output #9262
Conversation
Just change to tap13.
The test suite is semver-nothing. |
CI here: https://ci.nodejs.org/job/node-test-commit/5777/ This run is mostly about showing that the current jenkins tap parser accepts our tap13 output as well. |
@bnoordhuis not very happy with with change in gtest; "expected' also seems to be passed differently than through comment. Do you want to have a closer look or is this "good enough"? |
@mhdawson I recall you guys using this internally too, no? Do you have a different consumer than jenkins/tap? If so, can you try it? |
for (int i = 0; i < result.total_part_count(); ++i) { | ||
const TestPartResult& part = result.GetTestPartResult(i); | ||
OutputTapComment(stream, part.message()); | ||
if (result.total_part_count() > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The if
statement doesn't look like it's necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put it there to avoid writing "stack" unless used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I misread the diff that won't happen because it's inside the loop. If result.total_part_count() == 0
then i < result.total_part_count()
with i == 0
is false so it's not reached.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I messed up -- moved stuff around too quick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps should mention that i've fixed it since.
OutputTapComment(stream, part.message()); | ||
if (result.total_part_count() > 0) { | ||
for (int i = 0; i < result.total_part_count(); ++i) { | ||
*stream << " stack: |-\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what this is for. What does stack:
signify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's how we handle tracebacks from failed tests in the other test suite. It comes from how the tap13 producer in mocha wrote it. There seems to be these "almost defined" best practises for how traces should look in yamlish. Not saying its the right thing, but it is also how test.py output looks.
5e63d7e
to
fe70f13
Compare
ping @nodejs/build, @bnoordhuis (updated commits) |
@jbergstroem We use Jenkins tap as well, but we do also have a different consumer, we'll give it a go. |
So the new TAP output for a failing test seems to look like this:
Except for the first test, which looks like this:
|
@gibfahn I pass both stdout and stderr, similar to what the old runner did. I'm happy to drop stdout if people agree that's what we want. |
@jbergstroem I was talking about the |
fe70f13
to
0a83232
Compare
@gibfahn: Did you pull that from console or the tapfile? Here's a new run; doesn't seem to have the same issue: https://ci.nodejs.org/job/node-test-commit-jbergstroem-alpine34/33/nodes=test-joyent-alpine34-x64-2/console |
@jbergstroem It was from the console part of the tapfile, but I agree that it's not in the new run. It might be useful to put some basic info in the tapfile, such as the machine name, Jenkins job and number, workspace, Node commit hash, etc. If that info is in the tapfiles, then they become useful by themselves. Looking at the TAP13 spec it seems that having them with comments at the start of the tapfile should be fine. Is that something that might make sense? BackgroundWe are using a simple Node dashboard that parses the tapfiles and stores the results in a database. This lets you do things like look at when a test has failed in the past and see what machines it tends to fail on. We've found it really helpful for testing, and we'd be interested to see whether it's something that might be worth contributing. The server gets machine and node version info from the tapfiles. The dashboard was written by @BethGriggs. cc/ @mhdawson |
@gibfahn junit natively supports things like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The C++ changes LGTM but the python code looks like it still needs some work.
self.severity = 'fail' | ||
self.traceback = output.output.stdout + output.output.stderr | ||
|
||
# suggestions welcome |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is supposed to go here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reference to old code. Removed.
|
||
if output.HasCrashed(): | ||
self._printDiagnostic(PrintCrashed(output.output.exit_code)) | ||
self.severity = 'crashed' | ||
self.traceback = "oh no!\nexit code: " + PrintCrashed(output.output.exit_code) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long line.
@@ -316,6 +331,10 @@ def HasRun(self, output): | |||
# It should read as "duration including ms" rather than "duration in ms" | |||
logger.info(' ---') | |||
logger.info(' duration_ms: %d.%d' % (total_seconds, duration.microseconds / 1000)) | |||
if self.severity is not 'ok' or self.traceback is not '': | |||
if output.HasTimedOut(): | |||
self.traceback = 'timeout' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indent error.
@@ -89,6 +89,7 @@ deps/npm/node_modules/.bin/ | |||
tools/faketime | |||
icu_config.gypi | |||
test.tap | |||
cctest.tap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just ignore *.tap
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
@jbergstroem I was talking about adding it in the tap. Maybe if it was in the TAP then your junit converter could parse it into the native elements. It doesn't need to be in this PR though (obviously), we can raise it separately. |
@gibfahn: lets keep it out of this then. |
@bnoordhuis anything else? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you wanted me to do another review? LGTM.
@bnoordhuis your latest review status was "request for change". New CI here: https://ci.nodejs.org/job/node-test-commit/5956/ |
@nodejs/build: does anyone else want to chip in? This is taking us from tap to tap13. |
Rubber stamp LGTM |
Produce a tap13-compatible output which makes it simpler to parse. Output is still readable by the jenkins tap plugin. PR-URL: nodejs#9262 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Myles Borins <myles.borins@gmail.com>
This makes yaml-ish parsers happy. Note: gtest still seems to output the expected/result slightly different making the full traceback less informational. PR-URL: nodejs#9262 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Myles Borins <myles.borins@gmail.com>
We now have multiple tap producers; just ignore all files with the `.tap` extension. PR-URL: nodejs#9262 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Myles Borins <myles.borins@gmail.com>
Produce a tap13-compatible output which makes it simpler to parse. Output is still readable by the jenkins tap plugin. PR-URL: nodejs#9262 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Myles Borins <myles.borins@gmail.com>
This makes yaml-ish parsers happy. Note: gtest still seems to output the expected/result slightly different making the full traceback less informational. PR-URL: nodejs#9262 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Myles Borins <myles.borins@gmail.com>
We now have multiple tap producers; just ignore all files with the `.tap` extension. PR-URL: nodejs#9262 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Myles Borins <myles.borins@gmail.com>
Use yamlish instead of # to match community: nodejs/node#9262
Use yamlish instead of # to match community: nodejs/node#9262
Use yamlish instead of # to match community: nodejs/node#9262
Use yamlish instead of # to match community: nodejs/node#9262
Use yamlish instead of # to match community: nodejs/node#9262
Produce a tap13-compatible output which makes it simpler to parse. Output is still readable by the jenkins tap plugin. PR-URL: #9262 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Myles Borins <myles.borins@gmail.com>
This makes yaml-ish parsers happy. Note: gtest still seems to output the expected/result slightly different making the full traceback less informational. PR-URL: #9262 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Myles Borins <myles.borins@gmail.com>
We now have multiple tap producers; just ignore all files with the `.tap` extension. PR-URL: #9262 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Myles Borins <myles.borins@gmail.com>
Use yamlish instead of # to match community: nodejs/node#9262
Use yamlish instead of # to match community: nodejs/node#9262
Use yamlish instead of # to match community: nodejs/node#9262
This LTS release comes with 144 commits. This includes 47 that are docs related, 46 that are test related, 15 which are build / tools related, and 9 commits which are updates to dependencies Notable Changes: * buffer: - coerce slice parameters consistently (Sakthipriyan Vairamani (thefourtheye)) #9101 * deps: - *npm*: - upgrade npm to 3.10.9 (Kat Marchán) #9286 - *V8*: - Various fixes to destructuring edge cases - cherry-pick 3c39bac from V8 upstream (Cristian Cavalli) #9138 - cherry pick 7166503 from upstream v8 (Cristian Cavalli) #9173 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergström) #9262 * inspector: - inspector now prompts user to use 127.0.0.1 rather than localhost (Eugene Ostroukhov) #9451 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) #9586 PR-URL: #9735
This LTS release comes with 108 commits. This includes 30 which are doc related, 28 which are test related, 16 which are build / tool related, and 4 commits which are updates to dependencies. Notable Changes: The SEMVER-MINOR changes include: * build: - export openssl symbols on Windows making it possible to build addons linked against the bundled version of openssl (Alex Hultman) #7576 * debugger: - make listen address configurable in the debugger server (Ben Noordhuis) #3316 * dgram: - generalized send queue to handle close fixing a potential throw when dgram socket is closed in the listening event handler. (Matteo Collina) #7066 * http: - Introduce the 451 status code "Unavailable For Legal Reasons" (Max Barinov) #4377 * tls: - introduce `secureContext` for `tls.connect` which is useful for caching client certificates, key, and CA certificates. (Fedor Indutny) #4246 Notable SEMVER-PATCH changes include: * build: - introduce the configure --shared option for embedders (sxa555) #6994 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergström) #9262 * src: - node no longer aborts when c-ares initialization fails (Ben Noordhuis) #8710 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) #9586 PR-URL: #9736
This LTS release comes with 144 commits. This includes 47 that are docs related, 46 that are test related, 15 which are build / tools related, and 9 commits which are updates to dependencies Notable Changes: * buffer: - coerce slice parameters consistently (Sakthipriyan Vairamani (thefourtheye)) #9101 * deps: - *npm*: - upgrade npm to 3.10.9 (Kat Marchán) #9286 - *V8*: - Various fixes to destructuring edge cases - cherry-pick 3c39bac from V8 upstream (Cristian Cavalli) #9138 - cherry pick 7166503 from upstream v8 (Cristian Cavalli) #9173 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergström) #9262 * inspector: - inspector now prompts user to use 127.0.0.1 rather than localhost (Eugene Ostroukhov) #9451 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) #9586 PR-URL: #9735
This LTS release comes with 108 commits. This includes 30 which are doc related, 28 which are test related, 16 which are build / tool related, and 4 commits which are updates to dependencies. Notable Changes: The SEMVER-MINOR changes include: * build: - export openssl symbols on Windows making it possible to build addons linked against the bundled version of openssl (Alex Hultman) #7576 * debugger: - make listen address configurable in the debugger server (Ben Noordhuis) #3316 * dgram: - generalized send queue to handle close fixing a potential throw when dgram socket is closed in the listening event handler. (Matteo Collina) #7066 * http: - Introduce the 451 status code "Unavailable For Legal Reasons" (Max Barinov) #4377 * tls: - introduce `secureContext` for `tls.connect` which is useful for caching client certificates, key, and CA certificates. (Fedor Indutny) #4246 Notable SEMVER-PATCH changes include: * build: - introduce the configure --shared option for embedders (sxa555) #6994 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergström) #9262 * src: - node no longer aborts when c-ares initialization fails (Ben Noordhuis) #8710 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) #9586 PR-URL: #9736
This LTS release comes with 144 commits. This includes 47 that are docs related, 46 that are test related, 15 which are build / tools related, and 9 commits which are updates to dependencies Notable Changes: * buffer: - coerce slice parameters consistently (Sakthipriyan Vairamani (thefourtheye)) #9101 * deps: - *npm*: - upgrade npm to 3.10.9 (Kat Marchán) #9286 - *V8*: - Various fixes to destructuring edge cases - cherry-pick 3c39bac from V8 upstream (Cristian Cavalli) #9138 - cherry pick 7166503 from upstream v8 (Cristian Cavalli) #9173 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergström) #9262 * inspector: - inspector now prompts user to use 127.0.0.1 rather than localhost (Eugene Ostroukhov) #9451 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) #9586 PR-URL: #9735
This LTS release comes with 108 commits. This includes 30 which are doc related, 28 which are test related, 16 which are build / tool related, and 4 commits which are updates to dependencies. Notable Changes: The SEMVER-MINOR changes include: * build: - export openssl symbols on Windows making it possible to build addons linked against the bundled version of openssl (Alex Hultman) #7576 * debugger: - make listen address configurable in the debugger server (Ben Noordhuis) #3316 * dgram: - generalized send queue to handle close fixing a potential throw when dgram socket is closed in the listening event handler. (Matteo Collina) #7066 * http: - Introduce the 451 status code "Unavailable For Legal Reasons" (Max Barinov) #4377 * tls: - introduce `secureContext` for `tls.connect` which is useful for caching client certificates, key, and CA certificates. (Fedor Indutny) #4246 Notable SEMVER-PATCH changes include: * build: - introduce the configure --shared option for embedders (sxa555) #6994 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergström) #9262 * src: - node no longer aborts when c-ares initialization fails (Ben Noordhuis) #8710 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) #9586 PR-URL: #9736
This LTS release comes with 108 commits. This includes 30 which are doc related, 28 which are test related, 16 which are build / tool related, and 4 commits which are updates to dependencies. Notable Changes: The SEMVER-MINOR changes include: * build: - export openssl symbols on Windows making it possible to build addons linked against the bundled version of openssl (Alex Hultman) nodejs/node#7576 * debugger: - make listen address configurable in the debugger server (Ben Noordhuis) nodejs/node#3316 * dgram: - generalized send queue to handle close fixing a potential throw when dgram socket is closed in the listening event handler. (Matteo Collina) nodejs/node#7066 * http: - Introduce the 451 status code "Unavailable For Legal Reasons" (Max Barinov) nodejs/node#4377 * tls: - introduce `secureContext` for `tls.connect` which is useful for caching client certificates, key, and CA certificates. (Fedor Indutny) nodejs/node#4246 Notable SEMVER-PATCH changes include: * build: - introduce the configure --shared option for embedders (sxa555) nodejs/node#6994 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergstrom) nodejs/node#9262 * src: - node no longer aborts when c-ares initialization fails (Ben Noordhuis) nodejs/node#8710 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) nodejs/node#9586 PR-URL: nodejs/node#9736 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
This LTS release comes with 144 commits. This includes 47 that are docs related, 46 that are test related, 15 which are build / tools related, and 9 commits which are updates to dependencies Notable Changes: * buffer: - coerce slice parameters consistently (Sakthipriyan Vairamani (thefourtheye)) nodejs/node#9101 * deps: - *npm*: - upgrade npm to 3.10.9 (Kat Marchan) nodejs/node#9286 - *V8*: - Various fixes to destructuring edge cases - cherry-pick 3c39bac from V8 upstream (Cristian Cavalli) nodejs/node#9138 - cherry pick 7166503 from upstream v8 (Cristian Cavalli) nodejs/node#9173 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergstrom) nodejs/node#9262 * inspector: - inspector now prompts user to use 127.0.0.1 rather than localhost (Eugene Ostroukhov) nodejs/node#9451 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) nodejs/node#9586 PR-URL: nodejs/node#9735 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
This LTS release comes with 108 commits. This includes 30 which are doc related, 28 which are test related, 16 which are build / tool related, and 4 commits which are updates to dependencies. Notable Changes: The SEMVER-MINOR changes include: * build: - export openssl symbols on Windows making it possible to build addons linked against the bundled version of openssl (Alex Hultman) nodejs/node#7576 * debugger: - make listen address configurable in the debugger server (Ben Noordhuis) nodejs/node#3316 * dgram: - generalized send queue to handle close fixing a potential throw when dgram socket is closed in the listening event handler. (Matteo Collina) nodejs/node#7066 * http: - Introduce the 451 status code "Unavailable For Legal Reasons" (Max Barinov) nodejs/node#4377 * tls: - introduce `secureContext` for `tls.connect` which is useful for caching client certificates, key, and CA certificates. (Fedor Indutny) nodejs/node#4246 Notable SEMVER-PATCH changes include: * build: - introduce the configure --shared option for embedders (sxa555) nodejs/node#6994 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergstrom) nodejs/node#9262 * src: - node no longer aborts when c-ares initialization fails (Ben Noordhuis) nodejs/node#8710 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) nodejs/node#9586 PR-URL: nodejs/node#9736 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
This LTS release comes with 144 commits. This includes 47 that are docs related, 46 that are test related, 15 which are build / tools related, and 9 commits which are updates to dependencies Notable Changes: * buffer: - coerce slice parameters consistently (Sakthipriyan Vairamani (thefourtheye)) nodejs/node#9101 * deps: - *npm*: - upgrade npm to 3.10.9 (Kat Marchan) nodejs/node#9286 - *V8*: - Various fixes to destructuring edge cases - cherry-pick 3c39bac from V8 upstream (Cristian Cavalli) nodejs/node#9138 - cherry pick 7166503 from upstream v8 (Cristian Cavalli) nodejs/node#9173 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergstrom) nodejs/node#9262 * inspector: - inspector now prompts user to use 127.0.0.1 rather than localhost (Eugene Ostroukhov) nodejs/node#9451 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) nodejs/node#9586 PR-URL: nodejs/node#9735 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
Here's a work in progress that generates valid (ish -- haven't tested all kinds of stuff just yet) tap13 output from the test runner. I was initially thinking that we'd do a separate test runner (tap and tap13) but seeing how our current tap is invalid at best and this diff is easier to read I thought I'd post it in its current state for feedback/discussion.
Together with https://pypi.python.org/pypi/tap2junit we now generate junit that jenkins like.
Open issues/things to discuss:
do we split up tap vs tap13 (i prefer not)if not, is this a semver major or minor? hard to say with tooling; especially seeing how our tooling has been invalid for so long.I'll rebase/clean up once we converge on what to do. Hopefully we can get this in as quick as possible -- the build groups general theory is that this will Make Jenkins Great Again.