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

test: formatting skip messages for TAP parsing #2109

Closed

Conversation

thefourtheye
Copy link
Contributor

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

@brendanashworth brendanashworth added the test Issues and PRs related to the tests. label Jul 5, 2015
@jbergstroem
Copy link
Member

Nice. LGTM.

@Fishrock123
Copy link
Contributor

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
Copy link
Contributor Author

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

@jbergstroem
Copy link
Member

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
Copy link
Contributor

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

@bnoordhuis
Copy link
Member

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
Copy link
Contributor Author

@bnoordhuis Done! PTAL :-)

@bnoordhuis
Copy link
Member

@thefourtheye
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Contributor Author

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

@jbergstroem
Copy link
Member

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
Copy link
Contributor

@thefourtheye
Copy link
Contributor Author

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

@jbergstroem
Copy link
Member

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
Copy link
Contributor Author

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

@jbergstroem
Copy link
Member

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

@thefourtheye
Copy link
Contributor Author

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

@jbergstroem
Copy link
Member

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

@bnoordhuis
Copy link
Member

LGTM with a comment.

@@ -3,8 +3,8 @@

// FIXME add sunos support
if ('linux freebsd darwin'.indexOf(process.platform) === -1) {
console.error('Skipping test, platform not supported.');
process.exit();
console.log(`1..0 # Skipped: Unsupported platform [${process.platform}]`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.
@brendanashworth
Copy link
Contributor

@thefourtheye is this landable?

@thefourtheye
Copy link
Contributor Author

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

@jbergstroem
Copy link
Member

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

thefourtheye added a commit that referenced this pull request Jul 20, 2015
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]>
thefourtheye added a commit that referenced this pull request Jul 20, 2015
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
Copy link
Contributor Author

Thanks people. Landed in 79c865a and 69298d3

@thefourtheye thefourtheye deleted the test-fixing-skip-messages branch July 20, 2015 10:24
thefourtheye added a commit to thefourtheye/io.js that referenced this pull request Aug 2, 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.
thefourtheye added a commit that referenced this pull request Aug 3, 2015
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants