Hello Sebastian!

First of all, sorry for taking so long to review the issues in the TAP plug-in. I'll have a cycle to work on the plug-in soon and I'm filtering the issues.

Thanks for reporting this issue, and for attaching the test results. The public Jenkins with the plug-in installed will be very useful too.

> As you can see, for all failed (sub-)tests there are lines starting with "#" in the table instead of the number of the failing test and the other columns

Could you point a test with that is missing the failing test, please?

I'm looking at test-results/t9903-bash-prompt.sh, and it has 2 failing tests, and below the test result, there's the comment-diagnostic info. Probably I'm looking at the wrong part of the test results.

Thanks in advance!

Hi Bruno,

something (probably on my side) seems to have changed recently. The original issue I was talking about is as follows: If you go to [1] and search for "test-results/t0061-run-command.sh" on that web page you'll see that in the table for test "t0061-run-command" subtest number 2 is missing. Instead, you see a number of comments in the table. But if you look at the slightly more recent test run at [2] there suddenly is subtest number 2, called "run_command can run a command", in the table. I'm not sure what caused the change.

I think you shoud simply ignore all lines

Anyway, even when looking at the (currently) most recent build at [3] there are a number of issues / inconsistencies:

1) That page contains the string "TAP Test Result (130 failures null)", not sure why "null" is in there. However, the total number of 130 failed tests is correct, that's the same number I'm calculating in my Groovy post-build script. The Groovy script also generates the output below "The following test(s) failed" on that page, which is my reference for the list of failing tests.

2) Slightly below, there's the text "This build contains 628 TAP test sets. Click here for details" and "1 parse error(s)". Again I'm not sure what causes the parse error, whether it's a bug in the TAP plugin or a bug it Git's test suite output. In any case it would be nice to see the name of the (sub-)test where the error occurred here.

3) The "TAP Test Result" from 1) and the "Click here for details" from 2) link to [4] and [5] respectively. At first I was confused that the TAP plugin provides two detailed results pages, and I'm still not sure whether this makes sense.

4) On [4], clicking on any if the ">>>" or test links under "All Failed Tests" results in "Status Code: 404". Also, the list of "All Failed Tests" does not seem to be correct as it includes the tests marked as "TODO known breakage". Maybe you could add an option to ignore results of tests marked as TODO? I'm also a bit confused why our total failed test count of 130 matches mine, although you count "TODO known breakage" as failures and I don't.

5) On [5], test marked as "TODO known breakage" are also marked red, i.e. as failures, but again the total number of failed tests oddly is correct. But the number of skipped tetsts is not. The header says "628 files 9549 tests, 8967 ok, 130 not ok, 1 skipped, 0 Bail Out!", but as you can see from the yellow entries in the table there are far more than 1 skipped tests. Also, whenever there's a comment in the raw TAP file, you make it part of the table. I think the plugin should simply ignore lines starting with "#" in the input files and not include them into the table. A good example on that page is at "test-results/t5801-remote-helpers.sh" where you see a lot of comments which should not be visible at all, IMHO. The bottom of the page finally contains the details about parse errors, which is great. Maybe the "1 parse error(s)" from the overview page could link here? In any case I like this result page much better than [4], and I'd like to suggest to completely drop [4] and just fix the issues with [5] instead. This will also lead to less confusion for the user which TAP result page to look at.

I hope all of this does not overwhelm you Thank in advance for any fixes!

[1] https://qa.nest-initiative.org/view/msysGit/job/msysgit-mingwGitDevEnv-test/96/tapResults/
[2] https://qa.nest-initiative.org/view/msysGit/job/msysgit-mingwGitDevEnv-test/99/tapResults/
[3] https://qa.nest-initiative.org/view/msysGit/job/msysgit-mingwGitDevEnv-test/125/
[4] https://qa.nest-initiative.org/view/msysGit/job/msysgit-mingwGitDevEnv-test/125/tapTestReport/
[5] https://qa.nest-initiative.org/view/msysGit/job/msysgit-mingwGitDevEnv-test/125/tapResults/

Wow, great feedback, much appreciated Sebastian!

The plugin started after a discussion in the mailing list. At that time, I was working with Selenium + TestLink and needed TAP for extending my test results.

Thus, the plug-in was tailored more or less to my own needs, and with time and with Pull Requests and issues here in Jenkins JIRA, it became more clear how to make it more generic for other users.

One thing that some users were missing, was the ability to merge their test results (JUnit, TestNG, etc) with the TAP results. Then I added the "TAP Test Result" link to the left. That link uses Jenkins testing API (kind like an abstract for JUnit, the default test format in Jenkins).

> At first I was confused that the TAP plugin provides two detailed results pages, and I'm still not sure whether this makes sense.

And that's why we have two detailed results page :o) I'll comment on the other items later, time to get the train.

Thanks a lot!

Code changed in jenkins
User: Bruno P. Kinoshita
Path:
src/main/java/org/tap4j/plugin/TapTestResultAction.java
http://jenkins-ci.org/commit/tap-plugin/4b259118ffc024a99c4cedf1bc13bfdae993440f
Log:
JENKINS-17941 Fixing test diff string

Hello Sebastian! How's it going?

Fixed two other issues in tap-plugin, now working on this one again

> 1) That page contains the string "TAP Test Result (130 failures null)", not sure why "null" is in there. However, the total number of 130 failed tests is correct, that's the same number I'm calculating in my Groovy post-build script. The Groovy script also generates the output below "The following test(s) failed" on that page, which is my reference for the list of failing tests.

I didn't recall what that "null" was supposed to be, so I had to check Jenkins core source code. That's the diff string. It used the previous build (if it had TAP tests) to compute the diff. I didn't override the AbstractTestResultAction#getPreviousResult() method and it wasn't being able to find the previous builds. I believe it has been fixed in 4b259118ffc024a99c4cedf1bc13bfdae993440f.

Moving on to the next item...

Code changed in jenkins
User: Bruno P. Kinoshita
Path:
src/main/resources/org/tap4j/plugin/TapResult/index.jelly
http://jenkins-ci.org/commit/tap-plugin/22dff5c9899056a3df0ea69b2bc8ff17a1d0c4a8
Log:
JENKINS-17941 Adding link to view file that failed to be parsed

> 2) Slightly below, there's the text "This build contains 628 TAP test sets. Click here for details" and "1 parse error(s)". Again I'm not sure what causes the parse error, whether it's a bug in the TAP plugin or a bug it Git's test suite output. In any case it would be nice to see the name of the (sub-)test where the error occurred here.

I believe that information is available in the build summary (the "Click here" link points to that page). Not sure if we should add that information in the project page, since it can contain a long list when many TAP streams failed to be parsed. Perhaps it could be added as a feature... since other users could find useful to have a list of failed tests... or bail out'ed tests.

I believe the TAP Stream of that parser error is this one: https://qa.nest-initiative.org/view/msysGit/job/msysgit-mingwGitDevEnv-test/125/tapResults/contents?f=test-results/t9200-git-cvsexportcommit.sh

I think the plug-in was right to fail to parse it. However, I had to manually enter the file name in the browser URL to view the file that the plug-in failed to parse.

Commit 22dff5c9899056a3df0ea69b2bc8ff17a1d0c4a8 adds a link to that file.

Code changed in jenkins
User: Bruno P. Kinoshita
Path:
src/main/resources/org/tap4j/plugin/TapBuildAction/summary.jelly
src/main/resources/org/tap4j/plugin/TapResult/index.jelly
http://jenkins-ci.org/commit/tap-plugin/7cab81f801c34a106b12391e04615e0ad1df5347
Log:
JENKINS-17941 Modify tapResults text on the UI. Was Tap test results, now Extended TAP Test Results

> 3) The "TAP Test Result" from 1) and the "Click here for details" from 2) link to [4] and [5] respectively. At first I was confused that the TAP plugin provides two detailed results pages, and I'm still not sure whether this makes sense.

Got it, you're right indeed. Added a screenshot showing the fixed test diff and the new text in the project page (it was altered in the build page too).

It was Tap Test Result, and you had to click on a Click "here" link to go to the page. Now there's a link (mimicking the behaviour of the other test result), but the name was changed to Extended TAP Test Results. What do you think?

Screenshot https://issues.jenkins-ci.org/secure/attachment/24018/17941-1.png
Fixed in commit 7cab81f801c34a106b12391e04615e0ad1df5347

> 4) On [4], clicking on any if the ">>>" or test links under "All Failed Tests" results in "Status Code: 404". Also, the list of "All Failed Tests" does not seem to be correct as it includes the tests marked as "TODO known breakage". Maybe you could add an option to ignore results of tests marked as TODO? I'm also a bit confused why our total failed test count of 130 matches mine, although you count "TODO known breakage" as failures and I don't.

I spotted this bug while working on another issue few days ago, but I have a meeting within moments and can't look which commit is it in :-/ sorry. But in short, I removed the >>> link, since you can see the TAP test by clicking on its name. Hope that works for you too.

Broke item 5 into four parts. Please, find the comments below:

> 5) On [5], test marked as "TODO known breakage" are also marked red, i.e. as failures, but again the total number of failed tests oddly is correct. But the number of skipped tetsts is not. The header says "628 files 9549 tests, 8967 ok, 130 not ok, 1 skipped, 0 Bail Out!", but as you can see from the yellow entries in the table there are far more than 1 skipped tests.

Hmmm. Weird. I imported test-results/t0000-basic.sh into my dev-workspace (hope you don't mind it). And the plug-in successfully reported "7 skipped,"... Looking at the code, though, I found a bug in the TapResult#tally() method:

[snippet]
if (plan.isSkip()) {
this.skipped = testResults.size();
} else {
for (TestResult testResult : testResults) {
if (isSkipped(testResult))

else if (isFailure(testResult))

else

}
}
[/snippet]

[zooming-in]
if (plan.isSkip()) {
this.skipped = testResults.size();
}
[/zooming-in]

I hope you understand what I'm highlighting here. If the plan was marked with SKIP for any reason, the skip count would be reset. Say you had 1 test in that Test Set. Possibly, you would have 1 Skipped test being incorrectly displayed.

Normal tests running (after importing your TAP stream):
60 tests, 52 ok, 1 not ok, 7 skipped, 0 Bail Out!.

Running again, after adding a Test Set with 10 tests, with the Plan marks them all as SKIP.
70 tests, 52 ok, 1 not ok, 10 skipped, 0 Bail Out!.

After fixing the tally method (now it's: skipped += testResults.size()
70 tests, 52 ok, 1 not ok, 17 skipped, 0 Bail Out!.

Hopefully it'll fix your issue. I tried to find a plan with SKIP on your build... but there were too many files to look for.

> Also, whenever there's a comment in the raw TAP file, you make it part of the table. I think the plugin should simply ignore lines starting with "#" in the input files and not include them into the table. A good example on that page is at "test-results/t5801-remote-helpers.sh" where you see a lot of comments which should not be visible at all, IMHO.

Deal, I'll add a new feature while working on issue JENKINS-18703 or JENKINS-17804.

> The bottom of the page finally contains the details about parse errors, which is great. Maybe the "1 parse error(s)" from the overview page could link here? In any case I like this result page much better than [4], and I'd like to suggest to completely drop [4] and just fix the issues with [5] instead. This will also lead to less confusion for the user which TAP result page to look at.

Good idea mate. Added a <a name='parseErrors' /> just before the Parse errors section. Now if you type

http://localhost:8080/job/test-17941/3/tapResults/#parseErrors

in your browser URL you'll see the parse errors. I've also added a link the build page that goes direct to this section.

> In any case I like this result page much better than [4], and I'd like to suggest to completely drop [4] and just fix the issues with [5] instead. This will also lead to less confusion for the user which TAP result page to look at.

Thanks!!! I prefer that page too However, there are users that rely on that page. They have other tests (not always TAP, maybe TestNG, JUnit, or the alikes) and aggregate these other test results, with the TAP test results. So if you have 100 TAP tests, 20 JUnit tests, and 2 TestNG you can view it all on this page. That's why I'm reluctant to remove it, I hope you'll understand it.

Thanks for reporting with so many details. Having a public Jenkins instace with the links helped a lot in working on this issue, and your requests were all very good. I'm sure other users will benefit of your changes to the plug-in.

All the best, Bruno

Code changed in jenkins
User: Bruno P. Kinoshita
Path:
src/main/java/org/tap4j/plugin/TapResult.java
src/main/resources/org/tap4j/plugin/TapBuildAction/summary.jelly
src/main/resources/org/tap4j/plugin/TapResult/index.jelly
http://jenkins-ci.org/commit/tap-plugin/a0cf096db375ad5dc28c2d51bc3b209cb4d9e6c8
Log:
[FIXED JENKINS-17941] Added handy parseErrors link to the build page. Fixed skipped counter. Added more awesomeness to the plug-in, thanks to Sebastian Schuberth

Ops, just read your comment... I think I was following top-down your comments, and thus only added the parseErrors while reading item 5)... anyway, funny coincidence time to have lunch and... attend all the meetings o/

Let me know if you need anything else, feel free to re-open it later if I missed anything, or report new issues.

> It was Tap Test Result, and you had to click on a Click "here" link to go to the page. Now there's a link (mimicking the behaviour of the other test result), but the name was changed to Extended TAP Test Results. What do you think?

Thanks, I think that's much clearer than before. But still, for my personal taste, only having what is now called "TAP Extended Test Results" would be enough, and you could get rid of generating the "TAP Test Results" page at all. I like that colorful table much better than that other expandable / collapsible table

> Let me know if you need anything else, feel free to re-open it later if I missed anything, or report new issues.

Thanks a lot for your responsiveness and all the fixes! I'll test your improvements as soon as your plugin has a new release that I can install via Jenkins' update mechanism (I do not have admin access on that Jenkins so I need to wait for an "official" update of your plugin). I'll file new tickets in case I find more issues because this one is already getting a bit crowded

Thanks again!

No problem, keep posting interesting issues, and we'll make TAP plug-in even more interesting :o)

I'll cut a new release tonight from home, but I'm afraid some issues will take longer to fix. So I'll work on Jenkins plug-ins till next Friday, where we can cut another release of tap-plugin, if needed. Release early, release often.

Cheers

> Release early, release often.

Exactly Your efforts are highly appreciated!

Fixed in 1.11 (should be available through the update site in the next minute/hours)