test: formatting skip messages for TAP parsing#2109
Uh oh!
There was an error while loading. Please reload this page.
Conversation
jbergstroem commented Jul 5, 2015
Nice. LGTM. |
Fishrock123 commented Jul 6, 2015
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? |
thefourtheye commented Jul 6, 2015
@Fishrock123 You mean like a common ffunction with the template string? How can we know if the tap is enabled? |
jbergstroem commented Jul 6, 2015
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. |
Fishrock123 commented Jul 6, 2015
Eh, ok. LGTM |
bnoordhuis commented Jul 6, 2015
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. |
thefourtheye commented Jul 7, 2015
@bnoordhuis Done! PTAL :-) |
bnoordhuis commented Jul 7, 2015
thefourtheye commented Jul 7, 2015
@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? |
jbergstroem commented Jul 7, 2015
@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. |
thefourtheye commented Jul 7, 2015
@jbergstroem Hmmm, in Console output also, I couldn't see any logs, but only the TAP results (ok/not ok) |
jbergstroem commented Jul 7, 2015
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. |
Fishrock123 commented Jul 8, 2015
thefourtheyeforce-pushed the test-fixing-skip-messages branch from d29f7d8 to 8a9922dComparethefourtheye commented Jul 8, 2015
Okay, I rebased |
jbergstroem commented Jul 8, 2015
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. |
thefourtheye commented Jul 8, 2015
@jbergstroem#2129 is not the |
jbergstroem commented Jul 8, 2015
@thefourtheye ah, I'll just continue studying the art of not reading properly. |
thefourtheye commented Jul 8, 2015
@jbergstroem lol, no problem :-) Do we need a separate CI run for this? |
jbergstroem commented Jul 8, 2015
@thefourtheye might as well; they're just idling. https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/126/ |
bnoordhuis commented Jul 8, 2015
LGTM with a comment. |
thefourtheyeforce-pushed the test-fixing-skip-messages branch from 8a9922d to 555c226Compare
thefourtheyeforce-pushed the test-fixing-skip-messages branch from 555c226 to e8dec36Compare
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.
brendanashworth commented Jul 20, 2015
@thefourtheye is this landable? |
thefourtheye commented Jul 20, 2015
@brendanashworth We can land this anytime but the relevant changes are in #2130. Cc @jbergstroem |
jbergstroem commented Jul 20, 2015
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 <[email protected]> Reviewed-By: Johan Bergström <[email protected]>
This patch uses `return` statement to skip the test instead of using `process.exit` call. PR-URL: #2109 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Johan Bergström <[email protected]>
thefourtheye commented Jul 20, 2015
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 <[email protected]> Reviewed-By: Johan Bergström <[email protected]>
5 participants
This patch makes the skip messages consistent so that the TAP plugin
in CI can parse the messages properly. The format will be
cc @bnoordhuis