#335 Script for run tests in Basic role.
Merged 4 years ago by astepano. Opened 5 years ago by astepano.

@@ -0,0 +1,142 @@ 

+ #!/bin/bash -efu

+ 

+ # Called from standard-test-basic role to run basic tests

+ # This script is copied to test-environment.

+ 

+ debug() {

+     if [ -n "$DEBUG" ]; then

+         echo "$*" >&2

+     fi

+ }

+ 

+ msg_usage() {

+     cat << EOF

+ 

+ Run basic test.

+ 

+ Usage:

+ $PROG <options>

+ 

+ Options:

+ -h, --help              display this help and exit

+ -v, --verbose           turn on debug

+ -w, --workdir           test environment work dir. Directory for test execution.

+ -a, --artifactsdir      test environment dir to store artifacts

+ -t, --testname          name of the test

+ -c, --cmd               shell command to run the test

+ EOF

+ }

+ 

+ # Entry

+ 

+ PROG="${PROG:-${0##*/}}"

+ DEBUG="${DEBUG:-}"

+ STR_CMD="${STR_CMD:-}"

+ STR_DEBUG="${STR_DEBUG:-}"

+ STR_VERBOSE="${STR_VERBOSE:-}"

+ STR_WORKDIR="${STR_WORKDIR:-}"

+ STR_TEST_NAME="${STR_TEST_NAME:-}"

+ STR_ARTIFACTS_DIR="${STR_ARTIFACTS_DIR:-/tmp}"

+ 

+ # http://wiki.bash-hackers.org/howto/getopts_tutorial

+ opt=$(getopt -n "$0" --options "hvt:w:a:c:" --longoptions "help,verbose,cmd:,testname:,workdir:,artifactsdir:" -- "$@")

+ eval set -- "$opt"

+ while [[ $# -gt 0 ]]; do

+     case "$1" in

+         -t|--testname)

+             STR_TEST_NAME="$2"

+             shift 2

+             ;;

+         -c|--cmd)

+             STR_CMD="$2"

+             shift 2

+             ;;

+         -w|--workdir)

+             STR_WORKDIR="$2"

+             shift 2

+             ;;

+         -a|--artifactsdir)

+             STR_ARTIFACTS_DIR="$2"

+             shift 2

+             ;;

+         -v|--verbose)

+             DEBUG="-v"

+             shift

+             ;;

+         -h|--help)

+             msg_usage

+             exit 0

+             ;;

+         --)

+             shift

+             ;;

+         *)

+             msg_usage

+             exit 1

+     esac

+ done

+ 

+ # Entry

+ 

+ if [ -z "$STR_TEST_NAME" ] || [ -z "$STR_WORKDIR" ] || [ -z "$STR_CMD" ]; then

+     echo "Use: $PROG -h for help."

+     exit 0

+ fi

+ 

+ mkdir -p "$STR_WORKDIR"

+ mkdir -p "$STR_ARTIFACTS_DIR"

+ STR_WORKDIR="$(realpath "$STR_WORKDIR")"

+ STR_ARTIFACTS_DIR="$(realpath "$STR_ARTIFACTS_DIR")"

+ debug "Test: $STR_TEST_NAME"

+ debug "Command: $STR_CMD"

+ debug "Work dir: $STR_WORKDIR"

+ debug "Artifacts dir: $STR_ARTIFACTS_DIR"

+ 

+ # Up to this point any fail is considered as ci-sytem fail. Exit code != 0.

+ # Starting from this point and bellow any fail is considered as a test fail. Exit code == 0.

+ 

+ clean_exit() {

+     rc=$?;

+     trap - SIGINT SIGTERM SIGABRT EXIT # clear the trap

+     echo "Run test '$STR_TEST_NAME': done. Test's exit code: $rc"

+     # Exit code == 0, no matter of the test result.

+     # Close tee pipes

+     for pid in $(ps -o pid --no-headers --ppid $$); do

+         if [ -n "$(ps -p $pid -o pid=)" ]; then

+             kill -s HUP $pid

+         fi

+     done

+     # Return non-zero when test command not found

+     if [[ $rc -eq 127 ]]; then

+         echo "$STR_TEST_NAME (problem with test execution)" >&2

+     fi

+     local status="FAIL"

+     if [[ $rc -eq 0 ]]; then

+         status="PASS"

+     fi

+     local run_journal="$STR_ARTIFACTS_DIR/test.log"

+     echo "${status} $STR_TEST_NAME" >> "$run_journal"

+     if [ -e "$logfile_stdout" ]; then

+         local new_logfile_stdout="$(dirname "$logfile_stdout")/${status}_str_$(basename "$logfile_stdout")"

+         mv -f "$logfile_stdout" "$new_logfile_stdout"

+     fi

+     exit 0

+ }

+ trap clean_exit SIGINT SIGTERM SIGABRT EXIT

+ 

+ export PATH="$PATH:$STR_WORKDIR"

+ mkdir -p "$STR_ARTIFACTS_DIR"

+ logfile_stdout="$STR_ARTIFACTS_DIR/$(echo "$STR_TEST_NAME" | sed -e 's/\//-/g').log"

+ logfile_stderr="$STR_ARTIFACTS_DIR/$(echo "$STR_TEST_NAME" | sed -e 's/\//-/g')-err.log"

+ logfile_stdout="$(realpath "$logfile_stdout")"

+ logfile_stderr="$(realpath "$logfile_stderr")"

+ exec 3>&1 4>&2 1> >(tee -a "$logfile_stdout" >&3) 2> >(tee -a "$logfile_stderr" >&4)

+ cd "$STR_WORKDIR"

+ # Purpose to spawn new bash is to ignore -efu setting for current shell

+ # Test command: run-basic-test -w wodir -c 'false; echo 123; echo 333 >&2; touch "123 123"; exit 43' -t my_test -a logs -v

+ if [ -f "$STR_CMD" ]; then

+     chmod 0775 "$STR_CMD"

+ fi

+ bash -c "$STR_CMD"

+ # Explicit return code

+ exit $?

@@ -2,48 +2,12 @@ 

  

  - block:

    - name: Execute tests

-     shell: |

-       if [[ -z ${TEST} ]]; then

-           echo "FAIL: Test case name is not set" >> {{ remote_artifacts }}/test.log

-           exit

-       fi

-       if [[ -z ${TEST_DIR} ]]; then

-           echo "FAIL: Test directory for $TEST not found" >> {{ remote_artifacts }}/test.log

-           exit

-       fi

-       if [[ -z ${TEST_CMD} ]]; then

-           echo "FAIL: Does not know how to run $TEST" >> {{ remote_artifacts }}/test.log

-           exit

-       fi

-       log_file_name=$(echo $TEST | sed -e 's/\//-/g').log

-       logfile={{ remote_artifacts }}/str_${log_file_name}

-       exec 2>>$logfile 1>>$logfile

-       cd $TEST_DIR

-       #if command is a file make it executable

-       cmd="$(echo $TEST_CMD | awk '{print $1;}')"

-       if [ -f "$cmd" ]; then

-           chmod 0775 "$cmd"

-       fi

-       status="FAIL"

-       #execute the test

-       eval $TEST_CMD

-       if [ $? -eq 0 ]; then

-           status="PASS"

-       fi

-       echo "${status} $TEST" >> {{ remote_artifacts }}/test.log

-       # Add test status as prefix to test case log

-       mv ${logfile} {{ remote_artifacts }}/${status}_str_${log_file_name}

-     environment:

-     #Allow to use tests as a simple string which means the test is expected

-     #to be in a directory with same name and test script `runtest.sh`

-     #It is also possible to define the test as dictionary, in that case it is possible

-     #to change test directory and test command line parameters while test name is item key

-       TEST:

-         "{{ item if item.keys is not defined else (item.keys()|list)[0] }}"

-       TEST_DIR:

-         "{{ tenv_workdir }}/{{ item if item.keys is not defined else item[(item.keys()|list)[0]]['dir']|default((item.keys()|list)[0]) }}"

-       TEST_CMD:

-         "{{'./runtest.sh' if item.keys is not defined else item[(item.keys()|list)[0]]['run']|default('./runtest.sh')}}"

+     script:

+         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() }}

      with_items:

      - "{{ tests }}"

  

Tested with:

  • hosts: localhost
    roles:
  • role: standard-test-basic
    tags:
    • classic
      tests:
    • test one:
      dir: "tdir for test 1"
      run: XXXXXXXXX="1 2 3 4 " env
    • test 2:
      dir: tdir two
      run: true; false
    • test 3 three:
      dir: dir3
      run: echo -e "hello world\nnewline"; exit 111

Signed-off-by: Andrei Stepanov astepano@redhat.com

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

rebased onto ade7d5e99e98fd8353ba6c23eeabeb23a2f182d3

5 years ago

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

rebased onto 87379d7

5 years ago

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 87e1e81 fixes this pull-request

Pull-Request has been merged by astepano

4 years ago

Pull-Request has been merged by astepano

4 years ago

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