Closed Bug 335752 Opened 15 years ago Closed 14 years ago

Need a shorter version of all.sh for developers

Categories

(NSS :: Test, enhancement, P4)

3.11
enhancement

Tracking

(Not tracked)

RESOLVED FIXED
3.11.6

People

(Reporter: wtc, Assigned: slavomir.katuscak+mozilla)

References

Details

Attachments

(6 files, 3 obsolete files)

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.
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 ?
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.
Attached patch Diff of all.shSplinter Review
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)
Attachment #220530 - Flags: review?(wtchang)
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.)
Attachment #220530 - Flags: review?(wtchang) → review-
Attached patch Diff of all.shSplinter Review
Patch re-generated with -u option.
Attachment #222840 - Flags: review?(christophe.ravel.bugs)
Slavo, your patch looks good.  Thanks.  I'll let Christophe
do the formal review.
Attachment #222840 - Flags: review?(christophe.ravel.bugs) → review+
Priority: -- → P4
Comment on attachment 222840 [details] [diff] [review]
Diff of all.sh

Christophe, should you check it in to the tip?
Attachment #222840 - Flags: superreview+
Attachment #222840 - Flags: review+
Attachment #222840 - Flags: superreview+
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.
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.
Attachment #242639 - Flags: review+
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
The same as combination of previous 2 patches, but for NSS_3_11_BRANCH (just to prevent diversity in branches).
Attachment #243372 - Flags: superreview?(christophe.ravel.bugs)
Attachment #243372 - Flags: review?(alexei.volkov.bugs)
Attachment #243372 - Flags: review?(alexei.volkov.bugs) → review+
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.
Attachment #243372 - Flags: superreview?(christophe.ravel.bugs) → superreview+
Assignee: nobody → slavomir.katuscak
Status: NEW → ASSIGNED
Attachment #251179 - Flags: superreview?(nelson)
Attachment #251179 - Flags: review?(alexei.volkov.bugs)
The same version as in trunk.
Attachment #243372 - Attachment is obsolete: true
Attachment #251180 - Flags: superreview?(nelson)
Attachment #251180 - Flags: review?(alexei.volkov.bugs)
Blocks: 363827
Comment on attachment 251179 [details] [diff] [review]
Fix for previous patch (trunk)

sr=nelson for trunk.
Attachment #251179 - Attachment description: Fix for previous patch. → Fix for previous patch (trunk)
Attachment #251179 - Flags: superreview?(nelson) → superreview+
Comment on attachment 251180 [details] [diff] [review]
Diff of all.sh (for NSS_3_11_BRANCH)

sr=nelson for branch
Attachment #251180 - Flags: superreview?(nelson) → superreview+
Attachment #251179 - Flags: review?(alexei.volkov.bugs) → review+
Attachment #251180 - Flags: review?(alexei.volkov.bugs) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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.  
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: --- → 3.11.6
Attachment #254425 - Flags: superreview?(nelson)
Attachment #254425 - Flags: review?(alexei.volkov.bugs)
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. 
Attachment #254425 - Flags: superreview?(nelson) → superreview?(neil.williams)
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.
Attachment #254425 - Flags: review?(alexei.volkov.bugs) → review-
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.
Attachment #254425 - Flags: superreview?(neil.williams) → superreview+
> 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.
Attachment #254425 - Attachment is obsolete: true
Attachment #255237 - Flags: superreview?(neil.williams)
Attachment #255237 - Flags: review?(alexei.volkov.bugs)
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".
Attachment #255237 - Flags: superreview?(neil.williams) → superreview+
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.
Attachment #255237 - Attachment is obsolete: true
Attachment #256627 - Flags: superreview?(alexei.volkov.bugs)
Attachment #256627 - Flags: review?(nelson)
Attachment #255237 - Flags: review?(alexei.volkov.bugs)
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.
(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 on attachment 256627 [details] [diff] [review]
Improved version of previous patch.

r=nelson.  
We can always tweak this some more later if needed.
Attachment #256627 - Flags: review?(nelson) → review+
Attachment #256627 - Flags: superreview?(alexei.volkov.bugs) → superreview+
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
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.