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

TAP Plugin issues on Jenkins #453

Closed
gibfahn opened this issue Jul 22, 2016 · 21 comments
Closed

TAP Plugin issues on Jenkins #453

gibfahn opened this issue Jul 22, 2016 · 21 comments
Assignees

Comments

@gibfahn
Copy link
Member

gibfahn commented Jul 22, 2016

The Jenkins TAP plugin has an issue where large numbers of tap files massively slow down the build jobs. This is because the TAP plugin stores all the results in the main build.xml file, and thus has to parse them every time a page is loaded.

@jbergstroem has been looking at running the ci jobs with junit/xunit instead, and @thealphanerd has adapted citgm to use junit/xunit as well in nodejs/citgm#150.

However there are some downsides to junit, so it might be worth considering alternatives.

  1. We could try to fix the TAP plugin
    • The TAP plugin is pretty small, so it's probably worth taking a look, but I'm not sure how doable this is.
  2. We could switch to Junit/xUnit
    • This works great in Jenkins, but is a lot less easy to manually parse
    • We either stop supporting TAP (breaking change) or keep making sure we support both (pain).
  3. We could use TAP but not view it in Jenkins
    • We could use a simple dashboard to see the overall test results as opposed to using the built in viewer in Jenkins.
    • This would be more work, but would also allow us to write something that gives a better overview of the ci.
@MylesBorins
Copy link
Contributor

One other option would be to write a tap -> junit conversion utility and add that to CI. That way we can keep the tap output and still look at it but avoid the tap plugin

@jbergstroem
Copy link
Member

@thealphanerd if there already is something like that we shoudl probably go for it, but changing tools/test.py wouldn't be too hard; just a bit squeezed in time the coming week.

@gibfahn
Copy link
Member Author

gibfahn commented Jul 24, 2016

@thealphanerd Makes sense, maybe something like this: https://www.npmjs.com/package/tap-xunit

@MylesBorins
Copy link
Contributor

@gibfahn I've experimented with it. Would likely work better with the node testsuite than with the citgm output. Shouldn't be too hard to put together a poc on Monday

@gibfahn
Copy link
Member Author

gibfahn commented Jul 24, 2016

@thealphanerd Okay. For testing purposes it's useful to run the npm test suite, as that has more complex TAP (subtests etc.)

@MylesBorins
Copy link
Contributor

@gibfahn we don't actually have the npm test suite running in CI.

If you want to collaborate on that we can chat about it in #317

@jbergstroem
Copy link
Member

The more I've worked with this the more I'm convinced that we should fix the TAP plugin. Optimally, I'd like the tap plugin to just pass ok/fail/skip status as well as failed tests back to Jenkins (for storage). That way, we can make better use of the json api when we provide information to github.

@gibfahn
Copy link
Member Author

gibfahn commented Aug 30, 2016

@jbergstroem So your suggested fix would be for the TAP plugin to still store the TAP results in the build.xml file, but to only store some of the info?

@jbergstroem
Copy link
Member

@gibfahn you tell jenkins to pick up the junit file(s), after which it stores it (more efficiently) in build.xml.

@gibfahn
Copy link
Member Author

gibfahn commented Aug 30, 2016

@jbergstroem So the TAP plugin converts TAP -> junit and stores the junit in build.xml?

EDIT: So the TAP plugin should be converting the TAP to junit and directly storing the junit in the build.xml file.

@jbergstroem
Copy link
Member

@gibfahn it provides primitives for parsing tap which seems to be done every time build.xml files are read (hint: a lot) instead of once. there's a few issues over at jenkins issue trackers about this.

@gibfahn
Copy link
Member Author

gibfahn commented Sep 26, 2016

@thealphanerd How's the TAP -> Junit converter coming along? Is there something I could get my hands on to test?

I'd like to test this on nodejs/node#7867 (test-npm), as the npm test output is quite complex already. It'd also be useful to compare the converter output to the output of make test-npm with the reporter set to xunit.

@jbergstroem
Copy link
Member

Script that we should be running on all workers: https://github.com/jbergstroem/tap2junit

@joyeecheung
Copy link
Member

joyeecheung commented Jan 3, 2018

What's the progress on this? https://issues.jenkins-ci.org/browse/JENKINS-20212 is still open. I think for option 3 it's possible for the github bot to parse the TAP output and do something about it, or it can trigger another bot to do that on test completion.

EDIT: or we can try out https://github.com/jbergstroem/tap2junit

@gibfahn
Copy link
Member Author

gibfahn commented Jan 3, 2018

EDIT: or we can try out jbergstroem/tap2junit

We should do this. @jbergstroem had a job that was working using tap2junit, and it should be easy enough to get installed on all our machines, so it's just a question of adding it ansible, installing it, and then adding the tap2junit step to the jobs.

Obviously if someone would like to fix TAP in Jenkins that would be ideal, but we seem to have a scarcity of Java experts.

@gdams
Copy link
Member

gdams commented Aug 27, 2018

@gibfahn can this be closed now? AFAIK we are using tap2junit on CI now?

@gibfahn
Copy link
Member Author

gibfahn commented Aug 27, 2018

Worth checking to make sure we're using it in every node-test-commit subjob, otherwise yep, can be closed.

@refack
Copy link
Contributor

refack commented Aug 27, 2018

There is a little friction. Some machines have python2.6 (no good pip, so installing tap2junit is a PITA).
Also windows machines are harder to script. I would use the Jenkins script console, but it takes ages to initiate the first command).
There's also a bug in tap2junit that it doesn't understand this:

not ok 2137 addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-binary
  ---
  duration_ms: 41.541
  severity: crashed
  exitcode: -9
  stack: |-
  ...

as a fail

@gibfahn
Copy link
Member Author

gibfahn commented Aug 27, 2018

There's also a bug in tap2junit that it doesn't understand this:

That seems pretty serious, so we're getting green CI even though tests have failed?

@refack
Copy link
Contributor

refack commented Sep 12, 2018

There's also a bug in tap2junit that it doesn't understand this:

That seems pretty serious, so we're getting green CI even though tests have failed?

Yellow.

@sam-github
Copy link
Contributor

Folks, we switched to using tap2junit, so I think this can be closed. If I misunderstand, please reopen it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants