@bgoncalv please add extended explanation:
Bare commit without any reason is insufficient.
The should be recorder some background why this is important.
With respect to:

Before we had commit from @psss : that added that line:
https://pagure.io/standard-test-roles/c/820ab1fe

Also with respect to the issue:
https://pagure.io/standard-test-roles/issue/296

What experience did you have, that motivate to send this PR (I understand that there is an internal ticket, this should be self-sufficient, that explains this change).

The CI pipeline sends different topic in case of test or infra issue, in case the test exits with ERROR we are sending infra topic because the playbook exits with error, but for package maintainer is much harder to see what failed and he can think the pipeline failed because of infra outage instead of fixing the test.

@bgoncalv the info should be not in comment in discussion, but rather in commit :-D
That will be easier to track, like @psss did:

➜  standard-test-roles git:(master) ✗ git show 820ab1f
commit 820ab1fec0f341650ecfe83222a94d884596447b
Author: Petr Šplíchal <psplicha@redhat.com>
Date:   Tue May 14 13:56:45 2019 +0200
    Clearly show overall result of the testing
    Report role result task has been updated to show actual result of
    testing, which means: ERROR upon any test execution problem, FAIL
    when any test fails and PASS if there is at least one test passed.

@bgoncalv from me : ACK, I would get some feedback from @psss, in retrospect to his previous commits.

Does it mean that for error-ing beakerlib tests (e.g. failure in the setup phase which is checking that the rpm is installed on the box) we would get a failed result? I think this is also not good, because unsuccessful system under test preparation (which is basically an infrastructure error) will be presented as a test failure.

What about rather extending the error message format so that it could contain xunit as well? It would be optional (for cases when infra fails much earlier) but could be filed when error appears during beakerlib setup phase. @mvadkert, what do you think?

Right, failure in setup would not fail the playbook, the test case name in the logs would still contain ERROR instead of FAIL.

I'd prefer any fail during beakerlib or basic role execution to be reported as test failure. For example if a package can't be installed in setup phase it could be the package name is wrong, this is test failure not infra. Also the same for basic role if the test says to run a script that doesn't exist.

Anyway, if everyone prefers to leave the behavior as it is I'm okay with closing this PR :)

@psss

Does it mean that for error-ing beakerlib tests (e.g. failure in the setup phase which is checking that the rpm is installed on the box) we would get a failed result? I think this is also not good, because unsuccessful system under test preparation (which is basically an infrastructure error) will be presented as a test failure.

This is the problem: Error currently means infrastructure error. Something the user did not cause. IN case of these errors, these are caused by the user (faulty setup phase in the test). I am fine keeping the error state, but then we need to start distinguishing between infra ERROR and ERROR in test. Note that in xunit user will still see errors and failures distinguished. But I do not think in current overall state ERROR is correct as it means infra error.

What about rather extending the error message format so that it could contain xunit as well? It would be optional (for cases when infra fails much earlier) but could be filed when error appears during beakerlib setup phase. @mvadkert, what do you think?

I am not sure that will help to the end user. I am completely fine adding optional xunit to error state, but I disagree that the testset result should be ERROR.

I see. Seems that there are different expectations about what the error actually means. For me it is important that pass and fail are clean. I mean: If there is a fail it means the testing itself failed, but not any of the (many) preparations steps we need to do until the test environment is ready. Into this SUT preparation I count beakerlib setup phase as well. So if it fails there, I'd rather say: Something went wrong so we cannot say if the test passed or failed because the testing environment was not ready. Thus I would still call it an error.

so we need distinguish between test error and infra error. I see no other options.

Ss I said, the main problem is that error will end up on plate of the person who runs the test (CI system). And this error should be really just something for the user who added the test.

The simplest thing would be just to extend the complete test status with error state and properly describe it. Then we can work on interpreting in the dashboard in a reasonable way.

And with this PR the test cases will contain PASS, FAIL and ERROR prefix so if a failure in beakerlib setup phase will show the test case with ERROR, but we will not fail the playbook and report as infra issue.

@bgoncalv do I understand correctly:

We currently consider as Infra Error: If any Beaker test ends in ERROR?

The fix says from you says:
1. Continue running other tests.
2. Do not report beaker execution test as infra error.

If yes, then this this makes sense.

another detail on results.yml the result of the test case will be error and the pipeline has to be updated to handle this result as test failure, if the messages gets update to support test error just the pipeline will need to be changed.

Commit a8403c6c fixes this pull-request

another detail on results.yml the result of the test case will be error and the pipeline has to be updated to handle this result as test failure, if the messages gets update to support test error just the pipeline will need to be changed.

+1 Testing farm also needs this fix

@psss @bgoncalv I proposed adding error result to tests:
https://pagure.io/fedora-ci/messages/pull-request/86