335752 - Need a shorter version of all.sh for developers

Status: RESOLVED FIXED

(NSS :: Test, enhancement, P4)

Description

[Wan-Teh Chang]

all.sh now takes a long time on some machines.  For
example on my Windows XP SP2 machine (2.4 GHz Pentium
4, 1 GB RAM), all.sh (with ECC enabled) takes 42 minutes.
In contrast, all.sh in NSS 3.10 (without ECC) takes 13
minutes.

If a developer uses all.sh to verify his changes regularly,
we need a shorter version of all.sh that can finish in
10 minutes.

Comment 1

[Slavomir Katuscak]

Do you have any idea what can be optimized ? Or you would rather have some smaller version where will be tested only part of all tests ?

Comment 2

[Wan-Teh Chang]

Let's begin with timing how long each sub-test (cert.sh,
cipher.sh, ssl.sh, smime.sh, etc.) takes.  Then we can
see which ones we can optimize.  On the other hand, it
may be easier to have a shorter version that omits some
tests and runs some tests with fewer iterations.

Comment 3

[Slavomir Katuscak]

Attachment: Diff of all.sh

I have created patch which is solving 2 things:

1. Adding timestamps + total time measure for testing subscripts
2. Enables using TESTS variable from system environment (for example TESTS=cert ./all.sh will test only cert tests)

Comment 4

[Wan-Teh Chang]

Comment on attachment 220530 [details] [diff] [review]
Diff of all.sh

Hi Slavo,

Thanks for the patch and sorry I forgot to
review it earlier.

Please re-generate the patch with "cvs diff -u".
The -u option produces a "unified context" diff,
which is easier to review.

Please ask Jason or Christophe to review your
new patch.  (I need to concentrate on FIPS
validation.)

Comment 5

[Slavomir Katuscak]

Attachment: Diff of all.sh

Patch re-generated with -u option.

Comment 6

[Wan-Teh Chang]

Slavo, your patch looks good.  Thanks.  I'll let Christophe
do the formal review.

Comment 7

[Alexei Volkov]

Comment on attachment 222840 [details] [diff] [review]
Diff of all.sh

Christophe, should you check it in to the tip?

Comment 8

[Slavomir Katuscak]

I checked it to the tip.

Checking in all.sh;
/cvsroot/mozilla/security/nss/tests/all.sh,v  <--  all.sh
new revision: 1.21; previous revision: 1.20
done

It will be also good to have it in jes5, to prevent diversity in branches.

Comment 9

[Slavomir Katuscak]

There is a problem with time command on Windows, which causes that init.sh runs before every testings script and this caused that some tests are failing. Sorry it's my fault, I haven't tested it on Windows before.

I prepared new patch, where I removed time command and added printing of TIMESTAMPS to logfile. Will need review ASAP.

Comment 10

[Slavomir Katuscak]

Attachment: Fix for previous patch.

Comment 11

[Slavomir Katuscak]

Fix checked into the tip.

Checking in all.sh;
/cvsroot/mozilla/security/nss/tests/all.sh,v  <--  all.sh
new revision: 1.22; previous revision: 1.21
done

Comment 12

[Slavomir Katuscak]

Attachment: Diff of all.sh (for NSS_3_11_BRANCH)

The same as combination of previous 2 patches, but for NSS_3_11_BRANCH (just to prevent diversity in branches).

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 243372 [details] [diff] [review]
Diff of all.sh (for NSS_3_11_BRANCH)

r=nelson.  

I suggest one change:

>+TESTS=${TESTS-$tests}

Make that:

TESTS=${TESTS:-$tests}
             ^ note the added colon.

Comment 14

[Slavomir Katuscak]

Attachment: Fix for previous patch (trunk)

Comment 15

[Slavomir Katuscak]

Attachment: Diff of all.sh (for NSS_3_11_BRANCH)

The same version as in trunk.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 251179 [details] [diff] [review]
Fix for previous patch (trunk)

sr=nelson for trunk.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 251180 [details] [diff] [review]
Diff of all.sh (for NSS_3_11_BRANCH)

sr=nelson for branch

Comment 18

[Slavomir Katuscak]

Trunk:
Checking in all.sh;
/cvsroot/mozilla/security/nss/tests/all.sh,v  <--  all.sh
new revision: 1.23; previous revision: 1.22
done

Branch:
Checking in all.sh;
/cvsroot/mozilla/security/nss/tests/all.sh,v  <--  all.sh
new revision: 1.19.2.2; previous revision: 1.19.2.1
done

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

I'm reopening this bug.  The changes made so far are good, but IMO more 
changes are needed to satisfy this bug's objective.  Here are some:

1( The script ssl.sh now does MUCH more than test SSL.  It also contains 
extensive CRL testing.  It does many SSL connections whose purpose is 
not to test SSL, but to test detection of cert revocation by the cert
library, due to CRLs.  This GREATLY lengthens the time required to test
SSL.  IMO, all that CRL testing should be moved to another script, or
at least there should be a way to disable it in ssl.sh.  Specifically,
the sections of ssl.sh that produce output tables named:
   CRL SSL Client Tests - with ECC
   Cache CRL SSL Client Tests - with ECC
need to be made optional.  On by default, but avoidable.

2) ssl.sh now runs all the basic cipher suite tests twice, IINM, once with 
only the client side using "bypass", and again with only the server side 
using bypass.  The testing with client-side bypass should be optional.  

Comment 20

[Slavomir Katuscak]

Attachment: Implemented changes suggested in comment 19.

Comment 21

[Slavomir Katuscak]

Added 3 optional environment variables:

NSS_TEST_DISABLE_CRL
NSS_TEST_DISABLE_CLIENT_BYPASS
NSS_TEST_DISABLE_SERVER_BYPASS

Set to any value will disable its part of SSL tests. 

Comment 22

[Alexei Volkov]

Comment on attachment 254425 [details] [diff] [review]
Implemented changes suggested in comment 19.

>+    if [ -z "$NSS_TEST_DISABLE_CRL" ] ; then
>+        ssl_crl_ssl
>+        ssl_crl_cache
>+    else
>+        echo "$SCRIPTNAME: Skipping CRL Client Tests"
>+    fi

If we disable crl tests, we should disable crl generation
as well, that is in cert.sh as generated crls and accompanied certs are only used in ssl crl tests.

>+    if [ -z "$NSS_TEST_DISABLE_SERVER_BYPASS" ] ; then
>+        SERVER_OPTIONS="-B -s"
>+        CLIENT_OPTIONS=""
>+        BYPASS_STRING="Server Bypass"
>+    else
>+        echo "$SCRIPTNAME: Skipping Cipher Coverage - Server Bypass Tests"
>+    fi

if you disabled both bypassed, you wont run ssl tests at all. 
Need at least check for this and if it is the case run without bypass on both sides.

Also, may be making all.sh understand syntax like

export TESTS=ssl[nocrl, noclienbypass]

will be better.

Comment 23

[Neil Williams]

Comment on attachment 254425 [details] [diff] [review]
Implemented changes suggested in comment 19.

I think you're missing a call to ssl_run after bypass conditional stuff. (See Alexei's comment.) If you fix that I think you'll be good to go.

Comment 24

[Slavomir Katuscak]

Attachment: Added option to disable CRL generation + fixed missing ssl_run.

> If we disable crl tests, we should disable crl generation
> as well, that is in cert.sh as generated crls and accompanied certs are only
> used in ssl crl tests.

Improved to disable also crl generation when NSS_TEST_DISABLE_CRL defined.

> if you disabled both bypassed, you wont run ssl tests at all. 
> Need at least check for this and if it is the case run without bypass on both
> sides.

Do we need this ? When you disable both, it means that you don't want to run ssl_run at all (you want to have faster suite and you don't need ssl_run results). Otherwise you disable just one or none.

> 
> Also, may be making all.sh understand syntax like
> 
> export TESTS=ssl[nocrl, noclienbypass]
> 

This is more difficult to process and it's also not very clear with option to disable crl generation in cert.sh.

> I think you're missing a call to ssl_run after bypass conditional stuff. (See
> Alexei's comment.) If you fix that I think you'll be good to go.

Fixed.

Comment 25

[Neil Williams]

Comment on attachment 255237 [details] [diff] [review]
Added option to disable CRL generation + fixed missing ssl_run.

r=+, except I don't think, Slavo, that you work for Sun Labs. Your attribution should probably read "Sun Microsystems".

Comment 26

[Slavomir Katuscak]

Attachment: Improved version of previous patch.

This patch is improved version of previous patch, with new options requested by Alexei.

Normally ssl.sh tests CRL tests and ciphers tests.

There are 2 variables to disable these tests:
NSS_TEST_DISABLE_CRL - disable CRL tests (in both cert.sh and ssl.sh)
NSS_TEST_DISABLE_CIPHERS - disable ciphers tests

For ciphers there are 3 more options (applied only if NSS_TEST_DISABLE_CIPHERS not set):
NSS_TEST_DISABLE_BYPASS - disable both client and server bypass, tests ciphers coverage without bypass (normally there are not runned tests without bypass)
NSS_TEST_DISABLE_CLIENT_BYPASS - disable client bypass (will run only server bypass if not disabled)
NSS_TEST_DISABLE_SERVER_BYPASS - disable server bypass (will run only server bypass if not disabled)

If both NSS_TEST_DISABLE_CLIENT_BYPASS and NSS_TEST_DISABLE_SERVER_BYPASS are set, it's equivalent to NSS_TEST_DISABLE_CIPHERS.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 256627 [details] [diff] [review]
Improved version of previous patch.

I think NSS_TEST_DISABLE_BYPASS is redundant, but that's not a huge 
problem.  

This patch uses a shell programming construct that I've never seen
before in bourne/korn shell scripts.  It is:
   command || { command; command; } 
The thing that strikes me as unusual about it is the use of the 
curly braces.  Does tihs work on all our supported platforms? 
Have you tested in on Linux, Solaris, and Windows?  
If so, and it works on all those platforms, then r=nelson.
Please let me know and I will mark this patch's review accordingly.

I have a question about NSS_TEST_DISABLE_CIPHERS.  
Does it disable the individual cipher tests (that use tstclnt)? 
Does it disable the stress     cipher tests (that use strsclnt)?
Does it disable both?  
Does it disable client authentication tests?

It would be nice to be able to separately disable the individual
cipher tests and separately disable the stress tests.  
But that can be done after this patch is checked in.

Comment 28

[Slavomir Katuscak]

(In reply to comment #27)
> This patch uses a shell programming construct that I've never seen
> before in bourne/korn shell scripts.  It is:
>    command || { command; command; } 
> The thing that strikes me as unusual about it is the use of the 
> curly braces.  Does tihs work on all our supported platforms? 
> Have you tested in on Linux, Solaris, and Windows?  
> If so, and it works on all those platforms, then r=nelson.
> Please let me know and I will mark this patch's review accordingly.
>

This structure was there already before this patch. I just changed strings inside and added one more usage.
 
> I have a question about NSS_TEST_DISABLE_CIPHERS.  
> Does it disable the individual cipher tests (that use tstclnt)? 
> Does it disable the stress     cipher tests (that use strsclnt)?
> Does it disable both?  
> Does it disable client authentication tests?

It disables ssl_run() function. All these tests are tested from ssl_run() function, so it disables all of them.

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 256627 [details] [diff] [review]
Improved version of previous patch.

r=nelson.  
We can always tweak this some more later if needed.

Comment 30

[Slavomir Katuscak]

Trunk:
Checking in cert/cert.sh;
/cvsroot/mozilla/security/nss/tests/cert/cert.sh,v  <--  cert.sh
new revision: 1.38; previous revision: 1.37
done
Checking in ssl/ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.76; previous revision: 1.75
done

Branch:
Checking in cert/cert.sh;
/cvsroot/mozilla/security/nss/tests/cert/cert.sh,v  <--  cert.sh
new revision: 1.28.2.8; previous revision: 1.28.2.7
done
Checking in ssl/ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.61.2.10; previous revision: 1.61.2.9
done