test: formatting skip messages for TAP parsing by thefourtheye · Pull Request #2109 · nodejs/node · GitHub

This patch makes the skip messages consistent so that the TAP plugin
in CI can parse the messages properly. The format will be

1..0 # Skipped: [Actual reason why the test is skipped]

cc @bnoordhuis

Nice. LGTM.

Perhaps we should inform the tests if they are being run under tap so this can be configured in the common test helpers file per test run output type?

@Fishrock123 You mean like a common ffunction with the template string? How can we know if the tap is enabled?

I think the text is as readable in tap output as in plain text. The test runner knows what mode we're in, but it feels like an unnecessary complication. We should really start abstracting output if we care about this.

I think the text is as readable in tap output as in plain text. The test runner knows what mode we're in, but it feels like an unnecessary complication. We should really start abstracting output if we care about this.

Eh, ok. LGTM

LGTM. Maybe replace the process.exit calls with return statements while you're there, that should make reasoning about the test suite's remaining process.exit calls easier.

@bnoordhuis Done! PTAL :-)

LGTM. CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/120/

@bnoordhuis I have never seen TAP in action. But looking at the Extended TAP Results in the CI, the changes didn't make any difference I guess. What am I missing?

@thefourtheye you can see the output in the raw stdout logs. Not sure if jenkins registers skipped test as a part of the parsed output list.

@jbergstroem Hmmm, in Console output also, I couldn't see any logs, but only the TAP results (ok/not ok)

It looks like the test runner actually returns 'ok' on skip (well, return code 0). I'll have a look at this. This patch is still good to go though.

New CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/123/

Okay, I rebased master, pulled in @jbergstroem's #2129 and changed the process.exit to return and squashed the commits.

I'd suggest we keep the PR's separate. I probably have to update #2129 shortly (regex change) and neither PR relies on the other to work. No strong opinion about it though.

@jbergstroem #2129 is not the test.py changes, but the "missing crypto checks" which you landed earlier ;-)

@thefourtheye ah, I'll just continue studying the art of not reading properly.

@jbergstroem lol, no problem :-) Do we need a separate CI run for this?

@thefourtheye might as well; they're just idling. https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/126/

LGTM with a comment.

@bnoordhuis PTAL :-)

This patch makes the skip messages consistent so that the TAP plugin in CI can parse the messages properly. The format will be 1..0 # Skipped: [Actual reason why the test is skipped]

This patch uses `return` statement to skip the test instead of using `process.exit` call.

@thefourtheye is this landable?

@brendanashworth We can land this anytime but the relevant changes are in #2130. Cc @jbergstroem

I'm LGTM for landing this. #2130 just makes it "visible".

This patch makes the skip messages consistent so that the TAP plugin in CI can parse the messages properly. The format will be 1..0 # Skipped: [Actual reason why the test is skipped] PR-URL: #2109 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

This patch uses `return` statement to skip the test instead of using `process.exit` call. PR-URL: #2109 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

Thanks people. Landed in 79c865a and 69298d3

This is a followup of nodejs#2109. The tests which didn't make it in nodejs#2109, are included in this patch. The skip messages are supposed to follow the format 1..0 # Skipped: [Actual reason why the test is skipped] and the tests should be skipped with the return statement.

This is a followup of #2109. The tests which didn't make it in #2109, are included in this patch. The skip messages are supposed to follow the format 1..0 # Skipped: [Actual reason why the test is skipped] and the tests should be skipped with the return statement. PR-URL: #2290 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

brendanashworth added the test Issues and PRs related to the tests. label Jul 5, 2015

jbergstroem mentioned this pull request Jul 8, 2015

tools: expose skip output to test runner #2130

Closed

thefourtheye force-pushed the test-fixing-skip-messages branch from d29f7d8 to 8a9922d Compare July 8, 2015 06:00

thefourtheye force-pushed the test-fixing-skip-messages branch from 8a9922d to 555c226 Compare July 8, 2015 11:46

thefourtheye reviewed Jul 8, 2015
View reviewed changes

thefourtheye force-pushed the test-fixing-skip-messages branch from 555c226 to e8dec36 Compare July 8, 2015 13:40

thefourtheye added 2 commits July 8, 2015 19:08

test: formatting skip messages for TAP parsing

66a892e

This patch makes the skip messages consistent so that the TAP plugin in CI can parse the messages properly. The format will be 1..0 # Skipped: [Actual reason why the test is skipped]

test: changing process.exit to return while skipping tests

e8dec36

This patch uses `return` statement to skip the test instead of using `process.exit` call.

thefourtheye closed this Jul 20, 2015

thefourtheye deleted the test-fixing-skip-messages branch July 20, 2015 10:24

cjihrig mentioned this pull request Jul 24, 2015

Release Proposal: v2.5.0 #2239

Merged

thefourtheye mentioned this pull request Aug 2, 2015

test: fix messages and use return to skip tests #2290

Closed

thefourtheye mentioned this pull request Sep 10, 2015

test: add test-spawn-cmd-named-pipe #2770

Closed

test: formatting skip messages for TAP parsing by thefourtheye · Pull Request #2109 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: formatting skip messages for TAP parsing - #2109

test: formatting skip messages for TAP parsing#2109thefourtheye wants to merge 2 commits intonodejs:masternodejs/node:masterfrom thefourtheye:test-fixing-skip-messagesthefourtheye/io.js:test-fixing-skip-messagesCopy head branch name to clipboard

Conversation

thefourtheye commented Jul 5, 2015

Uh oh!

jbergstroem commented Jul 5, 2015

Uh oh!

Fishrock123 commented Jul 6, 2015

Uh oh!

thefourtheye commented Jul 6, 2015

Uh oh!

jbergstroem commented Jul 6, 2015

Uh oh!

Fishrock123 commented Jul 6, 2015

Uh oh!

bnoordhuis commented Jul 6, 2015

Uh oh!

thefourtheye commented Jul 7, 2015

Uh oh!

bnoordhuis commented Jul 7, 2015

Uh oh!

thefourtheye commented Jul 7, 2015

Uh oh!

jbergstroem commented Jul 7, 2015

Uh oh!

thefourtheye commented Jul 7, 2015

Uh oh!

jbergstroem commented Jul 7, 2015

Uh oh!

Fishrock123 commented Jul 8, 2015

Uh oh!

thefourtheye commented Jul 8, 2015

Uh oh!

jbergstroem commented Jul 8, 2015

Uh oh!

thefourtheye commented Jul 8, 2015

Uh oh!

jbergstroem commented Jul 8, 2015

Uh oh!

thefourtheye commented Jul 8, 2015

Uh oh!

jbergstroem commented Jul 8, 2015

Uh oh!

bnoordhuis commented Jul 8, 2015

Uh oh!

thefourtheye Jul 8, 2015

Choose a reason for hiding this comment

Uh oh!

brendanashworth commented Jul 20, 2015

Uh oh!

thefourtheye commented Jul 20, 2015

Uh oh!

jbergstroem commented Jul 20, 2015

Uh oh!

thefourtheye commented Jul 20, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

test: formatting skip messages for TAP parsing#2109
thefourtheye wants to merge 2 commits into
nodejs:masterfrom
thefourtheye:test-fixing-skip-messages