Reconfigured test-cmd to continue on failure

[stevekuznetsov]

As test-cmd test bucket sizes shrink and cross-test
dependencies are removed, it makes less and less
sense to fail once and for all when a test-cmd test
case fails. Instead, we know we can't run any more
tests in the same bucket as the failed test, but we
can try to run every other test bucket.

Signed-off-by: Steve Kuznetsov [email protected]

@liggitt @deads2k @smarterclayton @Kargakis PTAL

[liggitt]

depends... I want the merge job to fail as fast as possible. test jobs I could see allowing to continue

[stevekuznetsov]

I want the merge job to fail as fast as possible

Since we run our tests in parallel, you won't see any difference in runtime unless all of our test cases fail early and this one is still running... but since go test and ginkgo already have this behavior, I don't see that happening.

[stevekuznetsov]

conformance flakes: #9548 #9490

re[test]

[deads2k]

depends... I want the merge job to fail as fast as possible. test jobs I could see allowing to continue

We don't fail the test job when test-go hits its first failure. I want the job to fail, but I want the process to continue running to show me all the failures.

[stevekuznetsov]

integration flake on #10090

re[test]

[deads2k]

if @Miciah says this will still fail my test-cmd runs in the end, I like the change in concept.

[deads2k]

Without my bash manual, I don't know what this does. If this doesn't fail properly, we just lost all the tests. be sure

[stevekuznetsov]

From the manual the first important bit is:

7.3. Other Comparison Operators
string comparison
-n
    string is not null.

From the manual the other important bit is:

10.2. Parameter Substitution
Manipulating and/or expanding variables
${parameter-default}, ${parameter:-default}
    If parameter not set, use default.
    ${parameter-default} and ${parameter:-default} are almost equivalent. The extra : makes a difference only when parameter has been declared, but is null. 

Quick example to show what that means for us:

#!/bin/bash

set -o errexit
set -o nounset
set -o pipefail

# run the test with no variable set
echo -n "\$value unset    : "
if [[ -n "${value:-}" ]]; then
    echo "true"
else
    echo "false" # because the variable is unset, we default it to nothing (${var:-}) and the test fails
fi

# run the test with an empty variable
echo -n "\$value=''       : "
value=''
if [[ -n "${value:-}" ]]; then
    echo "true"
else
    echo "false" # because the variable is set but null, we default it to nothing (${var:-}) and the test fails
fi

# run the test with a non-empty variable
echo -n "\$value=\"test\"   : "
value="test"
if [[ -n "${value:-}" ]]; then
    echo "true" # because the variable is set and not null, the test passes
else
    echo "false"
fi

# output
# $value unset    : false
# $value=''       : false
# $value="test"   : true

So this code will set $failed to a value when a test fails, ergo the check here will fail.

[stevekuznetsov]

@liggitt @deads2k thoughts on this one?

[deads2k]

@liggitt @deads2k thoughts on this one?

I like the idea in the title. I don't speak bash very well, so I can't say with confidence that this does that.

[stevekuznetsov]

I can't say with confidence

I can. It's been tested locally, as well.

[deads2k]

I can. It's been tested locally, as well.

If @Miciah agrees

[smarterclayton]

This makes sense to me.

[Miciah]

Running the test in an if block means we will no longer call any EXIT trap handler that the test may have set up. Typically, a test sets an EXIT trap handler that invokes os::test::junit::reconcile_output, which invokes os::test::junit::declare_suite_end as necessary. Will this change not cause us to fail to ensure we always call os::test::junit::declare_suite_end after a failed test, or will our failing to do so not cause problems?

[Miciah]

On second thought, ${test} is a script which will run in a separate shell process, right? So that shell process will still exit and invoke its EXIT trap handler.

[stevekuznetsov]

$ cat test.sh 
#!/bin/bash
function my_trap() {
    echo "Inside of trap!"
}
trap my_trap EXIT
set -o errexit
grep

$ if ./test.sh; then echo "Success!"; else echo "Failure!"; fi
Usage: grep [OPTION]... PATTERN [FILE]...
Try 'grep --help' for more information.
Inside of trap!
Failure!

Seems to work fine -- I'm not familiar with the EXIT trap override you are talking about. Do you have a link for it?

[stevekuznetsov]

The placement inside of the if will fail to send ERR if the test fails, so set -o errexit and our ERR trap (the stacktrace) won't fire, but that's on purpose.

[stevekuznetsov]

@Miciah LGTM'ed in an IRC conversation @deads2k

[deads2k]

@stevekuznetsov removed the merge comment. Whenever the queue opens up, this is fine to go in.

[stevekuznetsov]

@smarterclayton this is ready for merge imo

[openshift-bot]

Evaluated for origin test up to db62c8a

[openshift-bot]

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11489/) (Base Commit: a799ce2)

[stevekuznetsov]

[merge]

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11489/) (Image: devenv-rhel7_5370)

[openshift-bot]

Evaluated for origin merge up to db62c8a