Bug 93379 - No FAIL/SKIP test-case scenario handling in run-test.sh in name-test directory.
Summary: No FAIL/SKIP test-case scenario handling in run-test.sh in name-test directory.
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review-
Keywords: patch
Depends on:
Blocks:
 
Reported: 2015-12-15 06:41 UTC by amit tewari
Modified: 2016-01-25 16:43 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Patch for handling Fail/Skip test-cases in run-test.sh script (855 bytes, patch)
2015-12-15 06:41 UTC, amit tewari
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description amit tewari 2015-12-15 06:41:29 UTC
Created attachment 120517 [details] [review]
Patch for handling Fail/Skip test-cases in run-test.sh script

dbus/test/name-test/run-test.sh script is used for running  test cases in name-test directory.
This script do not handle the FAIL/SKIP scenario in c_test() routine.
In case test-case fails it always shows PASS.

In the c_test() routine 
...
e=0
  echo "# running test $t"
  if ! "${DBUS_TOP_BUILDDIR}/libtool" --mode=execute $DEBUG "$DBUS_TOP_BUILDDIR/test/name-test/$t" "$@" >&2; then
    e=$?
    echo "# exit status $e"
  fi
  interpret_result "$e" "$t" "$@"
 ...

e is initialized as 0 here in c_test() i.e PASS exit scenario. 

========== 
if ! "${DBUS_TOP_BUILDDIR}/libtool" --mode=execute $DEBUG "$DBUS_TOP_BUILDDIR/test/name-test/$t" "$@" >&2 
==========
Currently When IF condition fails , we will be calling interpret_result() routine with default value of e=0 which is PASS scenario and hence incorrect PASS log will be shown on Console.

To fix this we need to assign last command exit status in variable 'e' always. 
Eg. We may handle the ELSE scenario for Fail/Skip test cases to print correct Test-case logs on Console.

I am attaching patch for review. 
Please let me know the response.

Thanks
Amit Tewari
Comment 1 Simon McVittie 2015-12-15 12:07:43 UTC
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.
Comment 2 amit tewari 2015-12-15 12:25:27 UTC
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
Comment 3 amit tewari 2015-12-15 13:24:22 UTC
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
Comment 4 Simon McVittie 2015-12-15 17:17:01 UTC
(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.
Comment 5 amit tewari 2015-12-16 03:18:26 UTC
(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
Comment 6 amit tewari 2016-01-21 09:39:13 UTC
Hello ,

Please let us know your response.

Thanks 

Amit Tewari
Comment 7 Simon McVittie 2016-01-25 15:35:40 UTC
(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.
Comment 8 Simon McVittie 2016-01-25 16:43:41 UTC
Fixed in git for 1.10.8 and 1.11.2, thanks.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.