Bug 93379

Comment on attachment 120517 [details] [review]
Patch for handling Fail/Skip test-cases in run-test.sh script

Review of attachment 120517 [details] [review]:
-----------------------------------------------------------------

::: test/name-test/run-test.sh
@@ +51,1 @@
>    if ! "${DBUS_TOP_BUILDDIR}/libtool" --mode=execute $DEBUG "$DBUS_TOP_BUILDDIR/test/name-test/$t" "$@" >&2; then

The script isn't correct, but I don't think your proposed change is correct either, because the ! operator would result in $e being 1 if the test script $t *succeeded*? I think it should probably be more like the Python test function, something like this:

e=0
echo "running test $t"
"${DBUS_TOP_BUILDDIR}/libtool" --mode=execute $DEBUG "$DBUS_TOP_BUILDDIR/test/name-test/$t" "$@" >&2 || e=$?
echo "# exit status $e"
interpret_result "$e" "$t" "$@"

The goal is that the first argument to interpret_result is 0 for success, 77 if every test-case was skipped, or something else for a failure.

If the test doesn't really distinguish between skipped and success, it's OK for it to return 0 instead of 77 when every test-case was skipped.

Hello ,
Yes you are correct.
I think in that case one simpler would be to remove if statement and simply run the test case and then preserver exit status .We can simply print PASS/FAIL status then.
  e=0
  echo "# running test $t"
  "${DBUS_TOP_BUILDDIR}/libtool" --mode=execute $DEBUG "$DBUS_TOP_BUILDDIR/test/name-test/$t" "$@" >&2
    e=$?
    echo "# exit status $e"
  interpret_result "$e" "$t" "$@"

this way all the cases can be handled.
Thanks
Amit Tewari

Hello 

I checked If else branch for bash - 
$? returns exit status of last command executed on shell so ! operator would result in $e being 0, if the test script $t *succeeded*..
So my patch will work correctly
Please verify.

Thanks
Amit Tewari

(In reply to amit tewari from comment #3)
> I checked If else branch for bash - 
> $? returns exit status of last command executed on shell so ! operator would
> result in $e being 0, if the test script $t *succeeded*..
> So my patch will work correctly

No, the ! operator is part of a pipeline, and $? is the exit status of the most recent foreground *pipeline*. See dash(1) or bash(1) for the jargon terms "command" and "pipeline": both shells use those terms consistently.

In these one-liners, "true" represents a successful test (exit 0), and "false" represents a failed test (exit 1):

$ if ! true; then echo "not true; $?"; else echo "true; $?"; fi
true; 1
$ if ! false; then echo "not false; $?"; else echo "false; $?"; fi
not false; 0

That was in dash, Debian's minimal POSIX /bin/sh, but bash produces the same results. You can see that $? is the exit status of the complete pipeline "! whatever". Note that 0 is success/true in shell scripts, unlike basically every other programming language.

So if we applied Attachment #120517 [details], interpret_result would get the wrong first argument in *both* cases: if the test $t passes, it would assign e=1, and if the test $t fails, it would assign e=0.

The code in Comment #1 or Comment #2 (they're basically equivalent) would do the right thing, though. The version in Comment #1 would work under "set -e", which test scripts should really be using, although this one currently doesn't.

The whole test/name-test/ directory is not particularly good, and eventually I'd like to make those tests more like the ones in test/ (Bug #92899), .

> This script do not handle the FAIL/SKIP scenario in c_test() routine.
> In case test-case fails it always shows PASS.

Is there any test in the name-test directory that fails or is entirely skipped in practice?

I agree that this is worth fixing, but I'd like to know how much damage it's doing.

(In reply to Simon McVittie from comment #4)
> (In reply to amit tewari from comment #3)
> I checked If else branch for
> bash - 
> $? returns exit status of last command executed on shell so !
> operator would
> result in $e being 0, if the test script $t *succeeded*..
>
> So my patch will work correctly

No, the ! operator is part of a pipeline,
> and $? is the exit status of the most recent foreground *pipeline*. See
> dash(1) or bash(1) for the jargon terms "command" and "pipeline": both
> shells use those terms consistently.

In these one-liners, "true" represents
> a successful test (exit 0), and "false" represents a failed test (exit 1):
> $ if ! true; then echo "not true; $?"; else echo "true; $?"; fi
true; 1
$ if
> ! false; then echo "not false; $?"; else echo "false; $?"; fi
not false; 0
> That was in dash, Debian's minimal POSIX /bin/sh, but bash produces the same
> results. You can see that $? is the exit status of the complete pipeline "!
> whatever". Note that 0 is success/true in shell scripts, unlike basically
> every other programming language.

Thanks for correcting me.


So if we applied Attachment #120517 [details]
> [details], interpret_result would get the wrong first argument in *both*
> cases: if the test $t passes, it would assign e=1, and if the test $t fails,
> it would assign e=0.

The code in Comment #1 or Comment #2 (they're
> basically equivalent) would do the right thing, though. The version in
> Comment #1 would work under "set -e", which test scripts should really be
> using, although this one currently doesn't.

The whole test/name-test/
> directory is not particularly good, and eventually I'd like to make those
> tests more like the ones in test/ (Bug #92899), .

> This script do not
> handle the FAIL/SKIP scenario in c_test() routine.
> In case test-case fails
> it always shows PASS.

Is there any test in the name-test directory that
> fails or is entirely skipped in practice?

I agree that this is worth
> fixing, but I'd like to know how much damage it's doing.


when I test all test case pass in name-test directory. 

But when I was testing my multithread server client test program for contribution to community , then I detected the issue with run-test.sh.
I believe it should be fixed so that even in case some configuration error on a user setup at least logs show PASS/FAIL correctly, even in case someone contributes some patch or new test cases he will be able to test it correctly else he may misinterpret the console logs.

code in Comment #2 would make code smaller and handle all scenario , please let me know if we fix using comment #2 or you seems to change the design of name-test similar to test directory.

Thanks
Amit Tewari

Hello ,

Please let us know your response.

Thanks 

Amit Tewari

(In reply to Simon McVittie from comment #1)
> e=0
> echo "running test $t"
> "${DBUS_TOP_BUILDDIR}/libtool" --mode=execute $DEBUG
> "$DBUS_TOP_BUILDDIR/test/name-test/$t" "$@" >&2 || e=$?
> echo "# exit status $e"
> interpret_result "$e" "$t" "$@"

I've applied effectively this, because it gets us a step closer to being able to "set -e" in that script.

Fixed in git for 1.10.8 and 1.11.2, thanks.