Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-17941

Parse errors with Git's TAP test suite

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      I'm running tests on Git for Windows that I compile myself. To do so I'm using the following command line insode the git/t directory:

      $ prove -j 5 -a test-results.tar.gz ./t[0-9]*.sh

      Please find the generated archive attached.

      I'm later unpacking the archive and using this iin the TAP plugin's "Test results" field:

      test-results/*.sh

      This results in the following page:

      https://qa.nest-initiative.org/view/msysGit/job/msysgit-mingwGitDevEnv-test/lastCompletedBuild/tapResults/

      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. As a result, the summary "8641 tests, 8204 ok, 1 not ok, 1 skipped, 0 Bail Out!." at the top of the page is wrong. I've manually extracted the correct summary to this page in the "The following test(s) failed" section:

      https://qa.nest-initiative.org/view/msysGit/job/msysgit-mingwGitDevEnv-test/lastCompletedBuild/

      As you can see, there are way more tests failing than the 1 test that the TAP plugin lists in "TAP Test Result (1 failure / ±0)" on the same page.

        Attachments

          Issue Links

            Activity

            Hide
            kinow Bruno P. Kinoshita added a comment -

            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!

            Show
            kinow Bruno P. Kinoshita added a comment - 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!
            Hide
            sschuberth Sebastian Schuberth added a comment -

            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/

            Show
            sschuberth Sebastian Schuberth added a comment - 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/
            Hide
            kinow Bruno P. Kinoshita added a comment -

            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!

            Show
            kinow Bruno P. Kinoshita added a comment - 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!
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            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

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - 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
            Hide
            kinow Bruno P. Kinoshita added a comment -

            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...

            Show
            kinow Bruno P. Kinoshita added a comment - 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...
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            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

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - 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
            Hide
            kinow Bruno P. Kinoshita added a comment -

            > 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.

            Show
            kinow Bruno P. Kinoshita added a comment - > 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.
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            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

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - 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
            Hide
            kinow Bruno P. Kinoshita added a comment - - edited

            > 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

            Show
            kinow Bruno P. Kinoshita added a comment - - edited > 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
            Hide
            kinow Bruno P. Kinoshita added a comment -

            > 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.

            Show
            kinow Bruno P. Kinoshita added a comment - > 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.
            Hide
            sschuberth Sebastian Schuberth added a comment -

            >> 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 agree that the information itself should not be duplicated on that page. What I was thinking about was more like adding an anchor like

            https://qa.nest-initiative.org/view/msysGit/job/msysgit-mingwGitDevEnv-test/125/tapResults/#parse_errors

            and linking the "1 parse error(s)" text on

            https://qa.nest-initiative.org/view/msysGit/job/msysgit-mingwGitDevEnv-test/125/

            to that anchor, so you can directly jump to that section of the TAP results.

            > 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.

            I agree that it's correct to report a parse error in this case. Thanks for adding that direct link to the offending file!

            Show
            sschuberth Sebastian Schuberth added a comment - >> 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 agree that the information itself should not be duplicated on that page. What I was thinking about was more like adding an anchor like https://qa.nest-initiative.org/view/msysGit/job/msysgit-mingwGitDevEnv-test/125/tapResults/#parse_errors and linking the "1 parse error(s)" text on https://qa.nest-initiative.org/view/msysGit/job/msysgit-mingwGitDevEnv-test/125/ to that anchor, so you can directly jump to that section of the TAP results. > 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. I agree that it's correct to report a parse error in this case. Thanks for adding that direct link to the offending file!
            Hide
            kinow Bruno P. Kinoshita added a comment -

            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))

            { skipped += 1; }

            else if (isFailure(testResult))

            { failed += 1; }

            else

            { passed += 1; }

            }
            }
            [/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.

            Show
            kinow Bruno P. Kinoshita added a comment - 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)) { skipped += 1; } else if (isFailure(testResult)) { failed += 1; } else { passed += 1; } } } [/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.
            Hide
            kinow Bruno P. Kinoshita added a comment -

            > 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

            Show
            kinow Bruno P. Kinoshita added a comment - > 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
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            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

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - 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
            Hide
            kinow Bruno P. Kinoshita added a comment -

            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.

            Show
            kinow Bruno P. Kinoshita added a comment - 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.
            Hide
            sschuberth Sebastian Schuberth added a comment -

            > 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

            Show
            sschuberth Sebastian Schuberth added a comment - > 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
            Hide
            sschuberth Sebastian Schuberth added a comment -

            > 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!

            Show
            sschuberth Sebastian Schuberth added a comment - > 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!
            Hide
            kinow Bruno P. Kinoshita added a comment -

            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

            Show
            kinow Bruno P. Kinoshita added a comment - 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
            Hide
            sschuberth Sebastian Schuberth added a comment -

            > Release early, release often.

            Exactly Your efforts are highly appreciated!

            Show
            sschuberth Sebastian Schuberth added a comment - > Release early, release often. Exactly Your efforts are highly appreciated!
            Hide
            kinow Bruno P. Kinoshita added a comment -

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

            Show
            kinow Bruno P. Kinoshita added a comment - Fixed in 1.11 (should be available through the update site in the next minute/hours)

              People

              Assignee:
              kinow Bruno P. Kinoshita
              Reporter:
              sschuberth Sebastian Schuberth
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: