EnricoMi / publish-unit-test-result-action

GitHub Action to publish unit test results on GitHub
Apache License 2.0
601 stars 178 forks source link

Where to report the filename and line number in JUnit? #377

Open carlescufi opened 1 year ago

carlescufi commented 1 year ago

The JUnit spec doesn't mention file name and line number where an error occurred (eg. as attributes of failure). Since this action seems to display annotations inline with the PR files changed (i.e. the diff), how can one provide the file name and line number where an error was detected (for example by a linter program).

EnricoMi commented 1 year ago

You are right, file name and line number is not part of any of the unofficial specs.

However, some test tools use file and line in <testcase> for this (e.g. pytest): https://github.com/EnricoMi/publish-unit-test-result-action/blob/43acb3d1372c257f97cc780bee85a17cbaf9d46e/python/test/files/junit-xml/pytest/junit.fail.xml#L5-L6

Note: this is not the line where an error (test failure) occurred but where the failing test is defined.

carlescufi commented 1 year ago

@EnricoMi thanks very much for the reply. Unfortunately, as you mention, this is defined in the testcase element and not in the failure one, which makes it hard to implement the usecase I have, which is to have a single test case with multiple failures, sort of like this:

<?xml version="1.0" encoding="utf-8"?>
<testsuites tests="1" failures="5" errors="0" time="0.0">
        <testsuite name="Compliance" tests="1" errors="0" failures="5" skipped="0" time="0">
                <testcase name="Checkpatch" classname="Guidelines">
                        <failure message="subsys/bluetooth/host/hci_core.c:269  do not use C99 // comments" type="error">
C99_COMMENTS: do not use C99 // comments
File:subsys/bluetooth/host/hci_core.c
Line:269</failure>
                        <failure message="subsys/bluetooth/host/hci_core.c:270  that open brace { should be on the previous line" type="error">
OPEN_BRACE: that open brace { should be on the previous line
File:subsys/bluetooth/host/hci_core.c
Line:270</failure>
                        <failure message="subsys/bluetooth/host/hci_core.c:275  space prohibited between function name and open parenthesis '('" type="warning">
SPACING: space prohibited between function name and open parenthesis '('
File:subsys/bluetooth/host/hci_core.c
Line:275</failure>
                </testcase>
        </testsuite>
</testsuites>

This is a usecase that seems to be supported by the JUnit/xunit spec.

See https://github.com/pytest-dev/pytest/issues/5891

which then links to:

https://github.com/jenkinsci/xunit-plugin/blob/ce58c5eb1db91d0d181935889a20cc42d2697b03/src/main/resources/org/jenkinsci/plugins/xunit/types/model/xsd/junit-10.xsd#L85-L105

or

https://github.com/pytest-dev/pytest/blob/f54ec30a6d7969eba6a2003b4409d33828cd406d/testing/example_scripts/junit-10.xsd#L88-L98

Which seems to allow multiple failures in a single testcase.

carlescufi commented 1 year ago

@EnricoMi note that my goal is to reuse your action for one of the CI checks that is currently annotating manually, unlike others in the open source project I work on, which already use your action

EnricoMi commented 1 year ago

I think multiple failures should work with the action. When the messages are different, it should show multiple failures for the same testcase.

Have you tried above file with the action?

carlescufi commented 1 year ago

I think multiple failures should work with the action. When the messages are different, it should show multiple failures for the same testcase.

That's excellent to hear, but the problem is the filename/line number in each failure (hence the original question in this thread). I would need a way to convey to your action that the failure includes this "metadata" so it can place the GitHub annotations correctly as per GitHub's format. Each failure would match to an error/warning. I am currently doing this sort of manually in the action that handles this today:

I store the failures in a "special" class that derives from junitparser.Failure: https://github.com/zephyrproject-rtos/zephyr/blob/c67666ae1bdfcb570457ab528b8d7de34fcaf29e/scripts/ci/check_compliance.py#L161-L169

and then print those as GitHub annotations: https://github.com/zephyrproject-rtos/zephyr/blob/c67666ae1bdfcb570457ab528b8d7de34fcaf29e/scripts/ci/check_compliance.py#L1114-L1121

Have you tried above file with the action?

No, I haven't yet, but it's great to know it should work, thanks!

EnricoMi commented 1 year ago

Nothing stops you from adding attributes to the <failure> tags:

<testcase name="Checkpatch" classname="Guidelines">
  <failure file="subsys/bluetooth/host/hci_core.c" line="269" message="do not use C99 // comments" type="error">
C99_COMMENTS: do not use C99 // comments
File:subsys/bluetooth/host/hci_core.c
Line:269</failure>
</testcase>

And then extend this section of junit.py: https://github.com/EnricoMi/publish-unit-test-result-action/blob/71ca27c4a9d74fa7ec2898304ee75909e4a57124/python/publish/junit.py#L219-L240

It looks like this action "supports" multiple failures per testcase by picking the most severe, and from those the first only. So your use case (multiple results per test case) is not fully supported.

What you want here is to create one UnitTestCase per result in case.result. Then, get_result, get_message, and get_content are not needed anymore:

        result=get_result(results),
        message=get_message(results),
        content=get_content(results),

Finally,

        test_file=case._elem.get('file'),
        line=int_opt(case._elem.get('line')),

has to use the file and line from result if it exists, and fall back to case if not.

Except for the last step, this looks required to fully support the spec.

But I am hesitating to do the last bit as that is a bespoke JUnit XML spec.

EnricoMi commented 1 year ago

@carlescufi have you continued to work on this?

carlescufi commented 1 year ago

@carlescufi have you continued to work on this?

Not yet @EnricoMi sorry. I will try to find some time for this during the summer and otherwise let you know.