1579448 - Multiple changes to NSPR tests, find the mimimum set of tests that currently work on all platforms, prepare to reenable CI in 2019.

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P1)

Description

[Kai Engert [:KaiE:]]

I've tried to run the NSPR tests today on Linux 64bit.

The multiwait test fails. I've gone back to NSPR 4.4 from 2 which is the oldest version that I can build on my current system. The multiwait test was already failing in 4.4 so I suggest we disable it.

One more test was failing: fdcach

I found that it has started to fail with NSPR 4.10.8, because of bug 573192 which had removed the stack based fd cache. We should remove the corresponding tests.

Jan, does that make sense to you?

What results do you get if you execute NSPR runtests.sh on your system, same or more failures?

Comment 1

[Kai Engert [:KaiE:]]

Attachment: 1579448-v1.patch

Comment 4

[Kai Engert [:KaiE:]]

There are two scripts to run the scripts, sh and pl.

Jan, which one do you usually use? I don't know why are there two. The README.TXT suggests that the .pl might be more powerful, and control the set of executed tests per platform, which is supposed to be defined in a file runtests.txt, but that txt file doesn't exist.

Comment 5

[Kai Engert [:KaiE:]]

Attachment: 1579448-v2.patch

I'm trying to run the NSPR tests in taskcluster.
It uses hardcoded filenames in /tmp/ which we should avoid. I've changed the code to use files in the local directory instead.

There are multiple ways to run the tests:

• a runtest.sh script
• a runtest.pl script
• a Makefile rule runtests

Because the Makefile attempts to run all tests, but the scripts only run a subset (because some tests are known to fail), we should change the Makefile target to execute one of the scripts. (It's best for me to run a make target from taskcluster).

I've chosen the .sh script, as that reports the failure correctly to the caller with an exit code, allowing the make target to detect the failure.

Comment 6

[Jan Beich]

Attachment: FreeBSD: build/run tests log

Changes after applying attachment 9091390 [details] [diff] [review]:

--- before
+++ after
@@ -2700,7 +2700,7 @@ dlltest			Passed
 dtoa			Passed
 errcodes			Passed
 exit			Passed
-fdcach			FAILED
+fdcach			Passed
 fileio			Passed
 foreign			Passed
 formattm			Passed
@@ -2729,7 +2729,6 @@ lockfile			Passed
 logfile			Passed
 logger			Passed
 many_cv			Passed
-multiwait			Passed
 nameshm1			Passed
 nblayer			Passed
 nonblock			Passed

Comment 7

[Jan Beich]

(In reply to Kai Engert (:kaie:) from comment #0)

The multiwait test fails. I've gone back to NSPR 4.4 from 2 which is the
oldest version that I can build on my current system. The multiwait test was
already failing in 4.4 so I suggest we disable it.

multiwait test passes on FreeBSD, even on 11.2. I can't test other BSDs atm.

One more test was failing: fdcach

Thanks for fixing it. Downstream Mikhail fixed and enabled a few more tests: forktest, nbconn, poll_err.
https://github.com/freebsd/freebsd-ports/blob/173ce3c7ca54/devel/nspr/files/patch-tests

(In reply to Kai Engert (:kaie:) from comment #4)

There are two scripts to run the scripts, sh and pl.

Only sh but also run library tests. I don't run tests regularly because it takes too long.
https://github.com/freebsd/freebsd-ports/blob/173ce3c7ca54/devel/nspr/Makefile#L40-L42

Comment 8

[Jan Beich]

Comment on attachment 9091390 [details] [diff] [review]
1579448-v2.patch

Review of attachment 9091390 [details] [diff] [review]:
-----------------------------------------------------------------

::: pr/tests/foreign.c
@@ +232,5 @@
> +#define TEMP_DIR "./tmp"
> +#if defined(WIN32)
> +            mkdir(TEMP_DIR);
> +#else
> +            mkdir(TEMP_DIR, 0700);

Why not replace with PR_MkDir to avoid conditional?

::: pr/tests/sema.c
@@ +8,5 @@
>  
>  #include <stdio.h>
>  
> +#define SEM_NAME1 "./tmp-foo.sem"
> +#define SEM_NAME2 "./tmp-bar.sem"

Omitting leading / (slash) may not work on _PR_HAVE_POSIX_SEMAPHORES platforms. See also _pr_ConvertSemName.
https://pubs.opengroup.org/onlinepubs/9699919799/functions/sem_open.html
https://docs.oracle.com/cd/E23823_01/html/816-5171/sem-open-3rt.html
http://nixdoc.net/man-pages/HP-UX/man2/sem_open.2.html

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:jcj, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 10

[Kai Engert [:KaiE:]]

Attachment: 1579448-v3.patch

Comment 11

[Kai Engert [:KaiE:]]

Attachment: nspr-runtest.patch

Comment 12

[Kai Engert [:KaiE:]]

Jan, thanks for your feedback and your suggestions. Using PR_Mkdir is great idea :)
I'm reverting the changes to semaphore naming.

I'd like to execute the NSPR tests as soon as possible, and while experimenting on win/mac/linux, unfortunately many tests are failing.

I didn't have time to investigate what's failing. I would like to enable all the tests that currently work as a baseline. Then, afterwards, we can try enabling the other tests one by one, and fix code as needed.

(FYI, several of my experiments can be found at https://treeherder.mozilla.org/#/jobs?repo=nss-try - some of the more recent ones print a debug log.)

I'm attach a new patch, that disables all the tests that currently fail on at least one platform.

Comment 13

[Kai Engert [:KaiE:]]

Attachment: 1579448-v4.patch

Can you live with this?

Comment 14

[Kai Engert [:KaiE:]]

Windows behavior is a bit erratic. As soon as I disable one test, some other test starts to fail... I just disabled "parent" in addition. I wonder if there'a s problem with concurrency, if more than one test runs on the same machine.

Comment 15

[Kai Engert [:KaiE:]]

Comment on attachment 9105972 [details] [diff] [review]
1579448-v4.patch

yes, I think it's about races for the same port.

I want to change the port numbers based on debug/opt and 32/64 bit target.

Comment 16

[Jan Beich]

Comment on attachment 9105972 [details] [diff] [review]
1579448-v4.patch

Review of attachment 9105972 [details] [diff] [review]:
-----------------------------------------------------------------

::: pr/tests/runtests.sh
@@ +1,1 @@
> +#!/bin/bash

On BSD systems Bash may not be installed by default. Default Bash location varies e.g., /usr/local/bin/bash on DragonFly, FreeBSD, OpenBSD; /usr/pkg/bin/bash on NetBSD. Both /usr/local and /usr/pkg are user-configurable via LOCALBASE variable (environment or make).

Consider changing to: #!/usr/bin/env bash

@@ +5,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +if test -z $1
> +then
> +  echo "usage $0 <path-to-dist>"

Missing : (colon) before "usage" and "$0". Also, can be simplified to

    : ${1:?"usage: $0 <path-to-dist>"}

@@ +11,5 @@
> +fi
> +
> +pushd $1/lib
> +ABS_LIB=`pwd`
> +popd

Why not keep POSIX compatibility? /bin/sh maybe faster than Bash e.g., on FreeBSD.

    cd $1/lib
    ABS_LIB=$PWD
    cd -

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/cd.html

Comment 17

[Jan Beich]

Attachment: FreeBSD: build/run tests log

pr/tests/parent sometimes fails on FreeBSD. Could be a race in the test or a bug in NSPR abstraction or a kernel bug. Compared to previous testing my CPU was 75% (or less) busy i.e., 6/8 logical cores dedicated to package building.

I've attached the log from a successful run with changes proposed in comment 16.

Comment 18

[Kai Engert [:KaiE:]]

(In reply to Jan Beich from comment #16)

Consider changing to: #!/usr/bin/env bash

well, let's stay with /bin/sh and use your suggestion below.

Missing : (colon) before "usage" and "$0". Also, can be simplified to

ok

: ${1:?"usage: $0 <path-to-dist>"}

I think that's difficult to read. I'd like to keep my simpler version.

cd $1/lib
ABS_LIB=$PWD
cd -

ok, with that we can keep /bin/sh

Comment 19

[Kai Engert [:KaiE:]]

Attachment: 1579448-v5.patch [checked in]

This patch changes the port numbers, and names of semaphores, based on debug/opt and 32/64 bit, and should be more resistent to parallel execution of tests on the same machine.

Comment 20

[Kai Engert [:KaiE:]]

https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=9f9a4a531b8e753601b89101b1d1a0ade88e4807

Comment 21

[Jan Beich]

Comment on attachment 9105989 [details] [diff] [review]
1579448-v5.patch [checked in]

Works fine on FreeBSD: tested 11.3 i386 and 13.0 amd64.

Comment 22

[Jan Beich]

Comment on attachment 9105989 [details] [diff] [review]
1579448-v5.patch [checked in]

Review of attachment 9105989 [details] [diff] [review]:
-----------------------------------------------------------------

::: pr/tests/runtests.sh
@@ +268,5 @@
>  	done
>  fi;
>  
> +if [ $rval -ne 0 ]; then
> +  cat ${LOGFILE}

Maybe bad UX outside of automation. When a test fails its name can be determined from the summary, so user can rerun the test manually and analyze its verbose output. However, verbose output doesn't annotate test failures, so printing large $LOGFILE may consume entire scroll buffer, leaving user guessing which test actually failed.

${PAGER:-cat} may help.

Comment 23

[Kai Engert [:KaiE:]]

(In reply to Jan Beich from comment #22)

Maybe bad UX outside of automation. When a test fails its name can be
determined from the summary, so user can rerun the test manually and analyze
its verbose output. However, verbose output doesn't annotate test failures,
so printing large $LOGFILE may consume entire scroll buffer, leaving user
guessing which test actually failed.

${PAGER:-cat} may help.

Good point.

Alternatively, maybe an environment variable to disable the cat?

Comment 24

[Kai Engert [:KaiE:]]

Attachment: 1579448-hide.patch

Comment 25

[Kai Engert [:KaiE:]]

Comment on attachment 9105989 [details] [diff] [review]
1579448-v5.patch [checked in]

https://hg.mozilla.org/projects/nspr/rev/d03855113364d4a9f182a9f820b1787fd1100364

Comment 26

[Kai Engert [:KaiE:]]

Jan, do you want the incremental patch?

Comment 27

[Kai Engert [:KaiE:]]

Had to disable ntioto test because it fails.
https://hg.mozilla.org/projects/nspr/rev/50c1448672f16e98895a256575bdc0a382d1c0a5

Note the failure occurred with a revision that DIDN'T have the change from bug 1586070 yet.

Comment 28

[Kai Engert [:KaiE:]]

Fixed, minus the pending change, which was optional and didn't get a review yet.

Comment 29

[Mike Hommey [:glandium]]

FWIW, multiwait passes on Debian.