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

Parse errors with Git's TAP test suite, part 2

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      This is a follow-up to JENKINS-17941, listing any remaining issues I still see with version 1.11 of the plugin. Note that I'm very grateful for the latest changes which have fixed all major issues, so I'm starting to also list rather minor / cosmetic issues here.

      1) On the first succeeding test run, i.e. when there is no previous run to compute a difference to, the "TAP Test Result" text say something like "(129 failures )", i.e. there is a superfluous whitespace after "failures ".

      2) In case of no parse errors like at [1] the anchor text on the "TAP Extended Test Results" page is empty. The source code looks like "<a name="parseErrors"></a> No parse errors found.". For consistency, I think it would still be nice to have the heading in there which is shown if there is a table with parse errors. I.e., I think the source code should look like "<a name="parseErrors"><h3>Parse errors</h3></a> No parse errors found.".

      3) The two links say "TAP Test Result" and "TAP Extended Test Results", but I think both should either use singular ("Result") or plural ("Results").

      4) Clicking on any of the links in the "All (Failed) Tests" table at [2] still results in error 404 for me.

      5) At [3], the title says "629 files 9549 tests, 8968 ok, 129 not ok, 452 skipped, 0 Bail Out!.", but I think just of either the exclamation mark or the dot at the end of the sentence would be enough

      6) If you search for "t7401-submodule-summary.sh" on [3] you'll see that for failed tests the table still contains a lot of code that should not be part of the table. There are more example on that page, just search for "# test_" to find them. Similarly, I think the lines like "# passed all 7 test(s)" should also not be part of the table. But that should come for free if you just strip any commenting lines starting with "#" from the raw TAP output before processing it.

      [1] http://mingwgitdevenv.cloudapp.net/job/mingwGitDevEnv-test/2/
      [2] http://mingwgitdevenv.cloudapp.net/job/mingwGitDevEnv-test/2/tapTestReport/
      [3] http://mingwgitdevenv.cloudapp.net/job/mingwGitDevEnv-test/2/tapResults/?

        Attachments

          Issue Links

            Activity

            sschuberth Sebastian Schuberth created issue -
            sschuberth Sebastian Schuberth made changes -
            Field Original Value New Value
            Link This issue is related to JENKINS-17941 [ JENKINS-17941 ]
            Hide
            sschuberth Sebastian Schuberth added a comment -

            Results of test run #2 attached.

            Show
            sschuberth Sebastian Schuberth added a comment - Results of test run #2 attached.
            sschuberth Sebastian Schuberth made changes -
            Attachment test-results.tar.gz [ 24107 ]
            Hide
            kinow Bruno P. Kinoshita added a comment -

            Thanks again Sebastian. Let's work together on this issue and include it in 1.12 (prob. Friday).

            Show
            kinow Bruno P. Kinoshita added a comment - Thanks again Sebastian. Let's work together on this issue and include it in 1.12 (prob. Friday).
            kinow Bruno P. Kinoshita made changes -
            Status Open [ 1 ] In Progress [ 3 ]
            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/a33b069d78c7d7ee3f508bb6a0b47f74a0cfb3f6
            Log:
            JENKINS-18885 use plural for test results actions display names

            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/a33b069d78c7d7ee3f508bb6a0b47f74a0cfb3f6 Log: JENKINS-18885 use plural for test results actions display names
            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/9a2e8af7619e9ca27b5116d33ecbe0f7dedc7a0b
            Log:
            JENKINS-18885 remove extra dot

            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/9a2e8af7619e9ca27b5116d33ecbe0f7dedc7a0b Log: JENKINS-18885 remove extra dot
            Hide
            sschuberth Sebastian Schuberth added a comment -

            Just to keep track, 3) and 5) are done

            Show
            sschuberth Sebastian Schuberth added a comment - Just to keep track, 3) and 5) are done
            Hide
            kinow Bruno P. Kinoshita added a comment -

            1) is on the way

            Show
            kinow Bruno P. Kinoshita added a comment - 1) is on the way
            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/9f1d0ef02b5dd6621f03631be5dea9241b87e19e
            Log:
            JENKINS-18885 adding empty tap result object

            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/9f1d0ef02b5dd6621f03631be5dea9241b87e19e Log: JENKINS-18885 adding empty tap result object
            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/7b23569143a58b3a5bcbc4f57661c7433bae2b45
            Log:
            JENKINS-18885 fixing parseErrors link location

            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/7b23569143a58b3a5bcbc4f57661c7433bae2b45 Log: JENKINS-18885 fixing parseErrors link location
            Hide
            sschuberth Sebastian Schuberth added a comment -

            1), 2), 3) and 5) are done!

            Show
            sschuberth Sebastian Schuberth added a comment - 1), 2), 3) and 5) are done!
            Hide
            kinow Bruno P. Kinoshita added a comment -

            6) I forgot about that! Sorry, that's doable. Will work on this later.

            4) hmmm-m-m... tricky. It's the same bug as JENKINS-17855 (reopened too). Caused due to subdirectories. Dumb me was testing only with TAP in the workspace, with no subdirectories. It may take a little longer... But I think I can work a little more on this issue today after lunch. Thanks!

            Show
            kinow Bruno P. Kinoshita added a comment - 6) I forgot about that! Sorry, that's doable. Will work on this later. 4) hmmm-m-m... tricky. It's the same bug as JENKINS-17855 (reopened too). Caused due to subdirectories. Dumb me was testing only with TAP in the workspace, with no subdirectories. It may take a little longer... But I think I can work a little more on this issue today after lunch. Thanks!
            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/TapBuildAction.java
            src/main/java/org/tap4j/plugin/model/TapStreamResult.java
            src/main/java/org/tap4j/plugin/model/TapTestResultResult.java
            http://jenkins-ci.org/commit/tap-plugin/21e7af07d5ae18327eb7a2626184970a6ff1d995
            Log:
            JENKINS-18885 and JENKINS-17855 fixing 404 URLs... round 2

            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/TapBuildAction.java src/main/java/org/tap4j/plugin/model/TapStreamResult.java src/main/java/org/tap4j/plugin/model/TapTestResultResult.java http://jenkins-ci.org/commit/tap-plugin/21e7af07d5ae18327eb7a2626184970a6ff1d995 Log: JENKINS-18885 and JENKINS-17855 fixing 404 URLs... round 2
            kinow Bruno P. Kinoshita made changes -
            Status In Progress [ 3 ] Open [ 1 ]
            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/AbstractTapProjectAction.java
            src/main/java/org/tap4j/plugin/TapParser.java
            src/main/java/org/tap4j/plugin/TapPublisher.java
            src/main/java/org/tap4j/plugin/TapResult.java
            src/main/resources/org/tap4j/plugin/TapPublisher/config.jelly
            src/main/resources/org/tap4j/plugin/tags/line.jelly
            src/test/java/org/tap4j/plugin/issue16647/TestIssue16647.java
            src/test/java/org/tap4j/plugin/issue16964/TestIssue16964.java
            http://jenkins-ci.org/commit/tap-plugin/295d37aee0ca1c0ac154b0820a215cce1648e01d
            Log:
            JENKINS-18885 add option to disable output of the TAP comment diagnostics

            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/AbstractTapProjectAction.java src/main/java/org/tap4j/plugin/TapParser.java src/main/java/org/tap4j/plugin/TapPublisher.java src/main/java/org/tap4j/plugin/TapResult.java src/main/resources/org/tap4j/plugin/TapPublisher/config.jelly src/main/resources/org/tap4j/plugin/tags/line.jelly src/test/java/org/tap4j/plugin/issue16647/TestIssue16647.java src/test/java/org/tap4j/plugin/issue16964/TestIssue16964.java http://jenkins-ci.org/commit/tap-plugin/295d37aee0ca1c0ac154b0820a215cce1648e01d Log: JENKINS-18885 add option to disable output of the TAP comment diagnostics
            Hide
            kinow Bruno P. Kinoshita added a comment -

            And now I think 6 and 4 are fixed! Marking issue as fixed, will cut a fix-release tonight. Probably you'll be able to give it a try tomorrow at noon.

            Cheers

            Show
            kinow Bruno P. Kinoshita added a comment - And now I think 6 and 4 are fixed! Marking issue as fixed, will cut a fix-release tonight. Probably you'll be able to give it a try tomorrow at noon. Cheers
            kinow Bruno P. Kinoshita made changes -
            Resolution Fixed [ 1 ]
            Status Open [ 1 ] Resolved [ 5 ]
            Hide
            kinow Bruno P. Kinoshita added a comment -

            Fixed in 1.12

            Show
            kinow Bruno P. Kinoshita added a comment - Fixed in 1.12
            kinow Bruno P. Kinoshita made changes -
            Status Resolved [ 5 ] Closed [ 6 ]
            Hide
            sschuberth Sebastian Schuberth added a comment -

            Thanks a lot, I can confirm all issues were addressed. There are just some very minor cosmetic issues left which I do not think justify to reopening this issues. But for the sake of completeness I'll mention them here:

            Regarding 2), the dot at the end of the sentence got lost: Instead of "No parse errors found." it now says "No parse errors found".

            Regarding 3), I now think the choice should probably have been to use singular ("Result") because that's what Jenkins itself is using in the "Latest Test Result" link.

            Regarding 4), the 404 is gone, but the table contains entries like "4 - - pretend we have a known breakage". Why are there two consecutive dashes? And the test number "4" is not very useful by itself without mentioning the test it belongs to ("t0000-basic.sh" in this example).

            Show
            sschuberth Sebastian Schuberth added a comment - Thanks a lot, I can confirm all issues were addressed. There are just some very minor cosmetic issues left which I do not think justify to reopening this issues. But for the sake of completeness I'll mention them here: Regarding 2), the dot at the end of the sentence got lost: Instead of "No parse errors found." it now says "No parse errors found". Regarding 3), I now think the choice should probably have been to use singular ("Result") because that's what Jenkins itself is using in the "Latest Test Result" link. Regarding 4), the 404 is gone, but the table contains entries like "4 - - pretend we have a known breakage". Why are there two consecutive dashes? And the test number "4" is not very useful by itself without mentioning the test it belongs to ("t0000-basic.sh" in this example).
            Hide
            kinow Bruno P. Kinoshita added a comment -

            2) Could you show me an example? Here I see only a <H?>Parse errors</H?> followed by No parse errors found, in the bottom of the TAP Test Results UI

            3) Hmmm, well... haha, we can fix it again later while working on other issues

            4) I think the consecutives dashes were already there, but without the test number.

            There was a block of code, where I was retrieving the test name. This block of code had a dumb bug, where I was doing something like return a == b ? bla bla + bla bla : bla bla... I don't recall exactly what was wrong, but it was never returning the desired value.

            Now it always return:

            <TEST_NUMBER> + SPACE + <TEST_DESCRIPTION>

            Being that the SPACE + <TEST_DESCRIPTION> part is added iff the TEST_DESCRIPTION is not blank. And TEST_DESCRIPTION is equivalent to the TAP Test Result name/description.

            IOW:

            ok 1 - How's it going?

            ok -> STATUS
            1 -> TEST_NUMBER

            • How's it going -> TEST_DESCRIPTION

            I'm not entirely happy with this approach, since sometimes it returns simply the test_number. Feel free to file another issue for this, but I'll probably take a look on it only in 1.14, 1.15 (Friday release is 1.13).

            Cheers, and thanks again for the feedback!

            Show
            kinow Bruno P. Kinoshita added a comment - 2) Could you show me an example? Here I see only a <H?>Parse errors</H?> followed by No parse errors found, in the bottom of the TAP Test Results UI 3) Hmmm, well... haha, we can fix it again later while working on other issues 4) I think the consecutives dashes were already there, but without the test number. There was a block of code, where I was retrieving the test name. This block of code had a dumb bug, where I was doing something like return a == b ? bla bla + bla bla : bla bla... I don't recall exactly what was wrong, but it was never returning the desired value. Now it always return: <TEST_NUMBER> + SPACE + <TEST_DESCRIPTION> Being that the SPACE + <TEST_DESCRIPTION> part is added iff the TEST_DESCRIPTION is not blank. And TEST_DESCRIPTION is equivalent to the TAP Test Result name/description. IOW: ok 1 - How's it going? ok -> STATUS 1 -> TEST_NUMBER How's it going -> TEST_DESCRIPTION I'm not entirely happy with this approach, since sometimes it returns simply the test_number. Feel free to file another issue for this, but I'll probably take a look on it only in 1.14, 1.15 (Friday release is 1.13). Cheers, and thanks again for the feedback!
            Hide
            sschuberth Sebastian Schuberth added a comment -

            2) That's exactly what I mean (you have to read the following text inside the quotes very carefully): Previously it was "No parse errors found." (there is a dot after "found") it now says "No parse errors found" (not dot after "found"). Yeah, I known I'm picky

            4) I really do not care deeply about this as I prefer the "TAP Extended Test Results" anyway. So from my side let's just keep is as-is.

            Show
            sschuberth Sebastian Schuberth added a comment - 2) That's exactly what I mean (you have to read the following text inside the quotes very carefully): Previously it was "No parse errors found." (there is a dot after "found") it now says "No parse errors found" (not dot after "found"). Yeah, I known I'm picky 4) I really do not care deeply about this as I prefer the "TAP Extended Test Results" anyway. So from my side let's just keep is as-is.
            Hide
            kinow Bruno P. Kinoshita added a comment -

            2) Ah! Got it, my bad, I changed that while fixing the other 'dot'

            Let me know if you need anything included in 1.13. So far I've labeled two minor issues to be included in this release.

            Show
            kinow Bruno P. Kinoshita added a comment - 2) Ah! Got it, my bad, I changed that while fixing the other 'dot' Let me know if you need anything included in 1.13. So far I've labeled two minor issues to be included in this release.
            rtyler R. Tyler Croy made changes -
            Workflow JNJira [ 150332 ] JNJira + In-Review [ 206812 ]

              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: