@psss @bgoncalv The promised PR :D
@psss I think, you are now unblocked to finish https://pagure.io/standard-test-roles/issue/296

@psss @bgoncalv please test before merge if you have time.

CI passed.

Thanks for the pull request, Andrei. I've tested it a bit and have a couple of comments:

The following line seems to be duplicated:

mkdir -p "$STR_ARTIFACTS_DIR"

When the shell command line contains semicolon only the first command is executed, for example:

- more:
    dir: .
    run: echo one; echo two; echo three; false

results in PASS and the output looks like this:

$ cat /tmp/artifacts/PASS-more.log
one
Run test 'more': done. Test's exit code: 0

As already mentioned in #327 I would vote for not separating stdout and stderr as it is causing unncessary clutter in the artifacts directory and makes it harder to quickly review failures. I don't see any advantage here.

Finally, if the test command is not found, I'd recommend reporting ERROR instead of FAIL to make it clear that the test execution was not successful.

There is an issue when the test fails. with official STR the test execution completes and create the FAIL_str_smoke.log, with the patch the playbook just aborts and not log is created.

standard-test-roles-3.2-1.fc29.noarch

TASK [str-common-final : Pull out the logs from test environment to test runner] **********************************************************************************************************************************
changed: [/root/rhel-guest-image-8.1.0.qcow2]
TASK [str-common-final : Report role result] **********************************************************************************************************************************************************************
fatal: [/root/rhel-guest-image-8.1.0.qcow2]: FAILED! => {
    "msg": [
        "Tests failed: True", 
        "Tests msg: FAIL smoke", 
        ""
    ]
}
    to retry, use: --limit @/udica/tests/tests.retry
PLAY RECAP ********************************************************************************************************************************************************************************************************
/root/rhel-guest-image-8.1.0.qcow2 : ok=22   changed=7    unreachable=0    failed=1
# ls artifacts/
default_provisioners.log  FAIL_str_smoke.log  rhel-guest-image-8.1.0.qcow2.guest.log  rhel-guest-image-8.1.0.qcow2.qemu.log  test.log

standard-test-roles-3.2-1.fc29.ade7d5e.1.noarch

TASK [standard-test-basic : Execute tests] ************************************************************************************************************************************************************************
fatal: [/root/rhel-guest-image-8.1.0.qcow2]: FAILED! => {"msg": "Unexpected templating type error occurred on (run-basic-test -v --workdir {{ tenv_workdir | regex_escape() }}/{{ item if item.keys is not defined else item[(item.keys()|list)[0]]['dir']|default((item.keys()|list)[0]) | regex_escape() }} --artifactsdir {{ remote_artifacts | regex_escape() }} --test {{ item if item.keys is not defined else (item.keys()|list)[0] | regex_escape() }} --cmd {{ './runtest.sh' if item.keys is not defined else item[(item.keys()|list)[0]]['run']|default('./runtest.sh') | regex_escape() }}): 'bool' object is not iterable"}
    to retry, use: --limit @/udica/tests/tests.retry
PLAY RECAP ********************************************************************************************************************************************************************************************************
/root/rhel-guest-image-8.1.0.qcow2 : ok=18   changed=3    unreachable=0    failed=1
# ls artifacts/
default_provisioners.log  rhel-guest-image-8.1.0.qcow2.guest.log  rhel-guest-image-8.1.0.qcow2.qemu.log

# cat tests.yml
- hosts: localhost
tags:
- classic
roles:
- role: standard-test-basic
    tests:
    - smoke:
        run: false

I also agree with @psss about not splitting the logs, but in case you want, would be possible to append the status to both logs? Now only stdout gets the status prefix. IMO it can be confusing to have PASS_smoke.log and smoke-error.log.

On this matter, before we have been requested to add _str prefix to the tests log names did you drop it intentionally?

@psss

# cat artifacts/FAIL-test\ one.log 
one
two
three
Run test 'test one': done. Test's exit code: 1
# cat test.yaml 
- hosts: localhost
  roles:
  - role: standard-test-basic
    tags:
    - classic
    tests:
    - test one:
        dir: tdir for test 1
        run: echo one; echo two; echo three; false

@psss about ERROR instead of FAIL - I agree, could you please do it in your PR?
My PR follows current behaviour. Current behaviour - doesn't have ERROR.

@psss

As already mentioned in #327 I would vote for not separating stdout and stderr as it is causing unncessary clutter in the artifacts directory and makes it harder to quickly review failures. I don't see any advantage here.

STDOUT - is buffered.
STDERR - is not buffered.

You will get clutter when you merge STDOUT + STDERR because messages will be mixed.
It will be hard to understand at which step you got error.

@psss
Merging STDERR + STDOUT comment from my friend:

"This is utter nonsense. stderr is destined to contain the failures without the "unnecessary clutter", so the absence of ability to quickly review it is a clear disadvantage and humongous impediment on a path to quick and effective issue triage process. Moreover, since stdout output is buffered by default, while stderr isn't, their merge often creates unreadable mess."

@bgoncalv added str to log filename.

I think I replied to all comments from @psss @bgoncalv . Please let me know if I can merge it.

@astepano On my comment I mentioned a couple of issues that I think were not addressed.

1. The first is a regression. The playbook got aborted instead of reporting failed test case. The test playbook is in the comment.

2. When log is created it creates PASS_str_smoke.log and smoke-err.log. I asked if wouldn't be better to be PASS_str_smoke-err.log.

@bgoncalv
1. You have bug in your playbook.

# cat tests.yml
- hosts: localhost
tags:
- classic
roles:
- role: standard-test-basic
    tests:
    - smoke:
        run: false  <------------------------ BUG

https://yaml.org/spec/1.2/spec.html

false is boolean in terms of yaml.

Also, please read:

Boolean conversion is helpful, but this can be a problem when you want a literal yes or other boolean values as a string. In these cases just use quotes:
non_boolean: "yes"
other_string: "False"

https://docs.ansible.com/ansible/latest/reference_appendices/YAMLSyntax.html

This, is not regression, this is rather a fix of existing bug.

1. Is not a blocker to merge, PR. We can tune-up PR endless. If you want to have PASS for STDERR - you will be able to send a PR. Currently I do not see, we it is needed.

okay then.
If it is just syntax error of my playbook, I still wonder why it didn't abort with official STR build though.

about the second topic, sure it is not a blocker, I just think it can be confusing. For example if there is a lot of logs on artifacts and they don't show close together. For example https://jenkins-continuous-infra.apps.ci.centos.org/view/Fedora%20All%20Packages%20Pipeline/job/fedora-f30-build-pipeline/229/artifact/package-tests/logs/

But sure, nothing here is a blocker feel free to merge if you want :)

Commit 87e1e81c fixes this pull-request

A couple of post-merge notes:

• The three-line example still broken for me (tested patch with the latest STR on Fedora)
• Line mkdir -p "$STR_ARTIFACTS_DIR" still duplicated
• Agree with @bgoncalv to use the same prefix for stdout/stderr files

I've also noticed that for beakerlib role there are now three output files created for each test. Is that expected?

I don't know anything about buffered or unbuffered, but the split made the logs totally unreadable.

See for example:

https://jenkins-continuous-infra.apps.ci.centos.org/job/fedora-f29-build-pipeline/536/artifact/package-tests/logs/PASS-str_smoke.log
https://jenkins-continuous-infra.apps.ci.centos.org/job/fedora-f29-build-pipeline/536/artifact/package-tests/logs/PASS-str_smoke-err.log

what is the output of what?

the decision whether the split or not split should not be made on technical problems, but usability. this is clearly not usable. please, force unbuffered on both a merge. it used to be like that and it worked nice.

see https://pagure.io/fedora-ci/general/issue/57