Closed Bug 462178 Opened 12 years ago Closed 12 years ago

Need better error handling when running NSPR tests (runtests.pl)

Categories

(NSPR :: NSPR, defect, P2)

4.7.2

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: christophe.ravel.bugs, Assigned: christophe.ravel.bugs)

Details

Attachments

(2 files, 2 obsolete files)

As commented in bug 461502, the current error handling in runtests.pl is confusing and sometimes wrong.
- Confusing:
    * Use "exit status" instead of "errno" which has a special meaning
    * Need to report kill signal if any
- Wrong:
    * use "$? >> 8" instead of "$? % 256" to parse exit code
Another potential issue I found: the current script does not handle correctly a failure to "exec" a NSPR test program. If the exec fails, the child process goes on and starts a new child with the rest of the list of programs to tests.
Attachment #345320 - Flags: superreview?(wtc)
Attachment #345320 - Flags: review?(slavomir.katuscak)
Priority: -- → P2
Comment on attachment 345320 [details] [diff] [review]
Address the issues listed in description and comment #1

r=wtc.

>+    # We should no be here unless exec failed.

"no" => "not"

>-    print_end($prog,$status);
>+    # There is no signal, no core on Windows
>+    print_end($prog,$status, 0, 0);

Nit: add a space after the first comma (,).
Attachment #345320 - Flags: superreview?(wtc) → superreview+
This patch was reviewed in bug 461502 as attachment 345113 [details] [diff] [review].
I checked it in on the NSPR trunk (NSPR 4.7.3).

Checking in runtests.pl;
/cvsroot/mozilla/nsprpub/pr/tests/runtests.pl,v  <--  runtests.pl
new revision: 1.2; previous revision: 1.1
done
Attachment #345320 - Flags: review?(slavomir.katuscak) → review+
The following code:

------
    if ($ltimeout == 0) {
        # we ran all the timeout: it's time to kill the child
        print "Timeout ! Kill child process $lpid\n";
        # Kill the child process and group
        kill(-9,$lpid);
        $lstatus = 1;
    }
------

produces the following output:
------
BEGIN TEST: nameshm1 (2008-09-29 21:27:09)

Timeout ! Kill child process 16335

END TEST: nameshm1 (2008-09-29 21:33:49)
TEST STATUS: nameshm1 = FAILED (exit status 0 - signal 1)
------

"signal 1" should be "signal 9". I will change "$lstatus = 1" to "$lstatus = 9".

In the same way, the following code:
------
    exec("./$lprog");
    # We should not be here unless exec failed.
    print "Faild to exec $lprog";
    exit 1;
------

will produce a "exit status 0 - signal 1". It should be "exit status 1" which is the exit status from the shell when one tries to run a program that doesn't exist.
I will change "exit 1" into "exit 256" (which gives 1 when >>8).
Attached patch Addresses comments #3 and #5. (obsolete) — Splinter Review
Attachment #345320 - Attachment is obsolete: true
Attachment #345524 - Flags: review?(wtc)
Comment on attachment 345524 [details] [diff] [review]
Addresses comments #3 and #5.

r=wtc.

>     # Start test program    
>     exec("./$lprog");
>+    # We should not be here unless exec failed.
>+    print "Faild to exec $lprog";
>+    exit 256;

It seems strange that "exit 1" in Perl doesn't cause the Perl
process to exit with status 1.  I'm not familiar with Perl, so
I'll trust you on this.

I suggest that you write "exit 256" as
    exit (1 << 8);
to make it clear that when we do "$status >> 8" later, we'll get
1.

>         print "Timeout ! Kill child process $lpid\n";
>         # Kill the child process and group
>         kill(-9,$lpid);
>-        $lstatus = 1;
>+        $lstatus = 9;

Ideally, we should call $ret = waitpid($lpid,0) again after
kill(-9,$lpid) to reap the test program and get the real exit
status.  For now, your code is fine.
Attachment #345524 - Flags: review?(wtc) → review+
Addresses comment #7. r+=wtc

Checking in runtests.pl;
/cvsroot/mozilla/nsprpub/pr/tests/runtests.pl,v  <--  runtests.pl
new revision: 1.3; previous revision: 1.2
Attachment #345524 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.