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: add junit output option to test runner #29840

Closed
wants to merge 3 commits into from

Conversation

richardlau
Copy link
Member

@richardlau richardlau commented Oct 4, 2019

Rather than output tap and then convert it via tap2junit into xml for our CI (to workaround bad performance uploading tap results to the builds) it might be more efficient to just output xml directly from the test runner. This PR attempts to add a junit output option to the test runner.

Things I can think of that still need looking at (hence currently WIP):

  • handling of flaky tests
  • check failure output is usable
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@richardlau richardlau added the wip Issues and PRs that are still a work in progress. label Oct 4, 2019
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Oct 4, 2019
@richardlau
Copy link
Member Author

This is sort of related to the ongoing discussion in nodejs/admin#413.

@nodejs-github-bot

This comment has been minimized.

@cclauss
Copy link
Contributor

cclauss commented Oct 4, 2019

This would be great! Or is someone knowledgeable could try out https://wiki.jenkins.io/display/JENKINS/TAP+Plugin

@richardlau
Copy link
Member Author

This would be great! Or is someone knowledgeable could try out https://wiki.jenkins.io/display/JENKINS/TAP+Plugin

@cclauss We used to use that plugin but ran into issues which is why we're now using tap2junit: nodejs/build#453

@rvagg
Copy link
Member

rvagg commented Oct 4, 2019

There's some manual tap output in various places, from memory various tests that skip they might be doing it manually. In CI we have some too, like in node-test-commit-linux-containered we do manual tap when confirming some things with Bash. Having follow-up JUint isn't going to work as well as follow-up tap so we're going to need to figure a way around that.

@rvagg
Copy link
Member

rvagg commented Oct 4, 2019

yeah, skips were consolidated into common.skip() in test/common/index.js printSkipMessage you'll see some tap. We'll need to pass down into common.js whether to print JUint or tap output for that.

@rvagg
Copy link
Member

rvagg commented Oct 4, 2019

oh, maybe this is parsing stdout from the tests so that doesn't matter? then we just need a solution for things like this in https://ci.nodejs.org/job/node-test-commit-linux-containered (and perhaps elsewhere)

if [ X"$(echo $OPENSSL_VERSION | grep 1\.1\.0)" = X"" ]; then
  FAIL_MSG="Not built with OpenSSL 1.1.0, exiting"
  echo $FAIL_MSG
  echo "1..1" > ${WORKSPACE}/test.tap
  echo "not ok 1 $FAIL_MSG" >> ${WORKSPACE}/test.tap
  exit -1
fi

@richardlau
Copy link
Member Author

yeah, skips were consolidated into common.skip() in test/common/index.js printSkipMessage you'll see some tap. We'll need to pass down into common.js whether to print JUint or tap output for that.

Skipping via common.skip() is handled:
https://github.com/nodejs/node/blob/d3e771040522b0c9db64fac23745f3f0556bfb9c/tools/test.py#L508-L512
and looks like this in Jenkins:
image

@richardlau
Copy link
Member Author

oh, maybe this is parsing stdout from the tests so that doesn't matter? then we just need a solution for things like this in https://ci.nodejs.org/job/node-test-commit-linux-containered (and perhaps elsewhere)

if [ X"$(echo $OPENSSL_VERSION | grep 1\.1\.0)" = X"" ]; then
  FAIL_MSG="Not built with OpenSSL 1.1.0, exiting"
  echo $FAIL_MSG
  echo "1..1" > ${WORKSPACE}/test.tap
  echo "not ok 1 $FAIL_MSG" >> ${WORKSPACE}/test.tap
  exit -1
fi

@rvagg Yeah I don't think this on its own will magic away completely the need for tap2junit. In addition to those examples unless this gets backported all the older release lines will still be spitting out tap from the test runner.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@richardlau
Copy link
Member Author

Since xml is not really human readable selecting junit currently outputs dots to stdout and then writes the xml at the end of the test run (the dots are to indicate something is actually happening and to allow some form of human readable output in the build logs). (n.b. we use dots on Travis.)

I remembered that we have some tooling in node-core-utils that parses job output so it would be less disruptive if we can still output tap to the job logs and write the JUnit xml to file.

@BridgeAR
Copy link
Member

What's the status here?

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@jasnell jasnell added the stalled Issues and PRs that are stalled. label Jun 25, 2020
@jasnell
Copy link
Member

jasnell commented Jun 25, 2020

Ping @richardlau

@richardlau
Copy link
Member Author

I'll close for now, I don't have the time to fix this up.

@richardlau richardlau closed this Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. stalled Issues and PRs that are stalled. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants