Closed Bug 398379 Opened 17 years ago Closed 17 years ago

Move shared DB tests from all.sh to another script file

Categories

(NSS :: Test, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

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

References

Details

Attachments

(5 files, 3 obsolete files)

All.sh contains many lines of code which doesn't have to be there. 

1. DB upgrade to shared db - I propose to have this in separate script file (for example tests/dbupgrade/dbupgrade.sh) and optional (not part of TESTS variable).
2. Setting many environment variables for shared db. Normally these variables are set in tests/common/init.sh script, I propose to have this in new script in common directory (for example tests/common/init-sharedb.sh). 

We can expect some more test complexity in future (running all.sh more times with different variables set), so I would like to keep all.sh as simple as possible.
Attached patch Proposed patch for all.sh only. (obsolete) — Splinter Review
Other scripts should be added.
Bob, it's your code, do you want to take this under your control ?
Shared DB tests should not be optional.

We eventually want to have full shared db tests turned on in tinderbox. If you want to make additional scripts, you should make sure those scripts automatically run all the other tests that would otherwise be run for all.sh.

Anyway Do NOT  remove anything until you have the new script available for review. 

Thanks,

bob
I agree with Bob's reply.  
The lines are present because they perform a required test purpose.

We could move them out of this script, into another script that is 
(say) sourced by this one, but that only adds to the overall complexity 
of the scripts, and that is not desirable.

Reducing complexity may be desirable.  Reducing functionality is not 
typically desirable.  Removing lines of the test script that perform
desired functionality is not desirable.  
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → WONTFIX
(In reply to comment #3)
> Shared DB tests should not be optional.

All other tests are optional (enabled by default). You can disable them by setting TESTS variable where you can specify which tests you want to run and then only these tests are tested. But it's not possible to disable Shared DB upgrade, these tests are tested in every run, also when it's not desirable (for example after memory leak checking).

(In reply to comment #4)
> I agree with Bob's reply.  
> The lines are present because they perform a required test purpose.

Yes, but in the different way than other tests. There should be no tests directly in all.sh.

> 
> We could move them out of this script, into another script that is 
> (say) sourced by this one, but that only adds to the overall complexity 
> of the scripts, and that is not desirable.

This is how we do it with all other tests. We use separate script for every testing area (cert.sh, ssl.sh,...). There is some logical directory structure for testing scripts called from all.sh, but these tests doesn't match it.
 
> Reducing complexity may be desirable.  Reducing functionality is not 
> typically desirable.  Removing lines of the test script that perform
> desired functionality is not desirable.  

It's not reducing functionality (all other tests can be disabled by setting TESTS variable with only tests you want to run). I only want to keep tests directory structure as it was and not to have tests directly in all.sh (similar to using functions instead of having everything in main() function).

If we plan to test all our tests with some feature enabled and then all the tests again with this feature disabled (we plan this with PKIX), we need to keep all.sh simple, otherwise it would cause big mess in this file.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
So the basic difference between this and other tests is that shared database is a characterist of all the tests (test with and without shared database). You don't just 'add shared db' tests, you run all the selected tests with the characteristics of using shared db.

This is analogous the bypass tests in SSL, we didn't add separate tests for bypass, we ran all the SSL tests under various forms of bypass.

bob
As I think about it, the issue you are probably running into is the database upgrade tests. They are required before we run the next stage of the dbtest testing.

It should probably be under the RUN_SHARED_DB_TESTS environment variable. I would be OK with that if we also make RUN_SHARED_DB_TESTS on by default.

bob
Bob, I understand that we run all the selected tests with characteristics of using shared db. But we want to run all the selected tests also with other options (PKIX,...) and putting all the work related to this directly to all.sh is not reasonable.

Before running tests scripts we call common/init.sh to set all paths and other variables and then we run these tests scripts by function run_tests().

Then there is upgrade to shared db directly in all.sh -> my proposal is to move this to another file and give user option to disable it (we don't need this for example in standard memory leak tests now, we don't tests memory leaks in db upgrade yet).

Then we set all path and other variables directly in all.sh -> my proposal is to move this to another file similar to common/init.sh like first time when these variables are set. 

Calling run_tests() again can stay in all.sh. 

Please check my patch (it's not final solution, just proposal) to see how I would like all.sh to look.
OK, before I r+ it, I would like to see the common/init-sharedb.sh and the updgradedb directory.

In addition your all.sh patch should change as follows:

1. leave the TABLE_ARGS in all.sh. Without the table args, it's very hard to identify which flavor of a particular test failed. Individual tests should not set this value, it should continue to be set by all.sh.

2. I would also like to see RUN_SHARED_DB_TESTS turned on by default (this is a pre-existing problem, but now that you have put the update half under the RUN_SHARED_DB_TESTS [which is reasonable], we need to make sure we have shared db coverage on tinderbox).

3. Finally, the original code that should be fixed: The upgrade db test section blindly sets TESTS to a set of tests. In should instead take the subset of tests specified and "tools fips sdr crmf smime ssl ocsp memleak".

bob
(In reply to comment #9)
> 1. leave the TABLE_ARGS in all.sh. Without the table args, it's very hard to
> identify which flavor of a particular test failed. Individual tests should not
> set this value, it should continue to be set by all.sh.

OK 

> 2. I would also like to see RUN_SHARED_DB_TESTS turned on by default (this is a
> pre-existing problem, but now that you have put the update half under the
> RUN_SHARED_DB_TESTS [which is reasonable], we need to make sure we have shared
> db coverage on tinderbox).

I checked the tests with this variable and found a problem that one test hangs in new DB tests:

cert.sh: Modify trust attributes of EC Root CA -t TC,TC,TC --------------------------
certutil -M -n TestCA-ec -t TC,TC,TC -d /Users/sven/nss/securitytip/mozilla/tests_results/security/apple.25/sharedb/server
Enter Password or Pin for "NSS Certificate DB":

After entering password manually tests continues. I can't enable shared DB tests until this is fixed.

> 
> 3. Finally, the original code that should be fixed: The upgrade db test section
> blindly sets TESTS to a set of tests. In should instead take the subset of
> tests specified and "tools fips sdr crmf smime ssl ocsp memleak".

Will be fixed in patch.
 

Attached patch Patch v2. (obsolete) — Splinter Review
This patch simplifies all.sh much. I need review ASAP, because I need to add another changes in all.sh (enable libpkix for all tests from TESTS).

I can't enable RUN_SHARED_DB_TESTS variable now, until problem from last comment is fixed, temporary solution what I can do is to put dbupgrade to default TESTS variable.
Assignee: nobody → slavomir.katuscak
Attachment #283333 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #284174 - Flags: review?(rrelyea)
Slavo, do you know the command line option to tell certutil to take its 
password from a file?
Nelson, I know that it's one-line fix in cert.sh:

-      certu -M -n "TestCA-ec" -t "TC,TC,TC" -d ${PROFILEDIR}
+      certu -M -n "TestCA-ec" -t "TC,TC,TC" -d ${PROFILEDIR} -f "${R_PWFILE}"

I'm more surprised that when we run this test without shared DB it passes and with shared DB it hangs.
Blocks: 396598
Summary: All.sh needs to be simplified. → Move shared DB tests from all.sh to another script file
Shared DB requires the password to update trust now. The old db does not.

I had already made the change to the other trust updates, but missed the ECC case.

bob
Attachment #284174 - Flags: review?(rrelyea)
No longer blocks: 396598
Attachment #284174 - Attachment is obsolete: true
Finally I implemented part of these changes as patch to bug 396598 and I'm waiting for the review from Alexei. I'm marking this bug as a blocker, because we need changes from this bug also there. After patch from 396598 is checked into CVS, I will prepare new patch for this bug.
Depends on: 396598
Blocks: 399681
See comments 10 and 13.
Attachment #285384 - Flags: review?(rrelyea)
Moved DB upgrade functionality to separate script, which is called from all.sh. 
Script is written in the same format as other testing script called from all.sh.
This patch also fixes problem from bug 399681.
Attachment #285386 - Flags: review?(rrelyea)
Moving functionality from all.sh:
DB upgrade -> moved to dbupgrade.sh
Setting path variables -> moved to common/init.sh (function init_directories)

There are some variables which need to be changed for PKIX tests or Shared DB tests and then changed back. Alexei suggested to move those variables to some file and then source it. For this I created function env_backup in common/init.sh.

> 2. I would also like to see RUN_SHARED_DB_TESTS turned on by default (this is a
> pre-existing problem, but now that you have put the update half under the
> RUN_SHARED_DB_TESTS [which is reasonable], we need to make sure we have shared
> db coverage on tinderbox).

I renamed this variable to NSS_TEST_DISABLE_SHARED_DB. With this name shared DB tests are enabled by default and variable need to be set only for disable. Also naming is similar to variable used to disable PKIX tests (NSS_TEST_DISABLE_PKIX).

Finally I cleaned up syntax in all.sh.
Attachment #285389 - Flags: review?(rrelyea)
Attachment #285384 - Flags: review?(rrelyea) → review+
Attachment #285386 - Flags: review?(rrelyea) → review+
Comment on attachment 285389 [details] [diff] [review]
Moving functionality from all.sh.

r-

It looks generally good, but the db upgrade isn't correct. The dbupgrade script is correct because it is explicitly using the sql: prefix, but the test runs with the updated database is not, the test runs need to have the 	NSS_DEFAULT_DB_TYPE environment variable set to "sql" before you run it, or the tests will continue to run against the old database.


bob
Attachment #285389 - Flags: review?(rrelyea) → review-
Checking in cert/cert.sh;
/cvsroot/mozilla/security/nss/tests/cert/cert.sh,v  <--  cert.sh
new revision: 1.47; previous revision: 1.46
done
RCS file: /cvsroot/mozilla/security/nss/tests/dbupgrade/dbupgrade.sh,v
done
Checking in dbupgrade/dbupgrade.sh;
/cvsroot/mozilla/security/nss/tests/dbupgrade/dbupgrade.sh,v  <--  dbupgrade.sh
initial revision: 1.1
done
Blocks: 400845
Improved previous patch. NSS_DEFAULT_DB_TYPE is set before run_tests. I also splitted shared db testing to 2 parts which can be disabled with 2 different variables NSS_TEST_DISABLE_UPGRADE_DB and NSS_TEST_DISABLE_SHARED_DB.
We need this because testing takes more time and we need options to split it to smaller parts (see bug 400845).
Attachment #285389 - Attachment is obsolete: true
Attachment #285894 - Flags: review?(rrelyea)
Comment on attachment 285894 [details] [diff] [review]
Moving functionality from all.sh - v2.

OK, this one looks good.
Thanks Slavo.

bob
Attachment #285894 - Flags: review?(rrelyea) → review+
Checking in all.sh;
/cvsroot/mozilla/security/nss/tests/all.sh,v  <--  all.sh
new revision: 1.40; previous revision: 1.39
done
Checking in common/init.sh;
/cvsroot/mozilla/security/nss/tests/common/init.sh,v  <--  init.sh
new revision: 1.55; previous revision: 1.54
done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
Attached patch Fix to all.shSplinter Review
In my last commit I replaced unset IOPR_HOSTADDR_LIST by disabling libpkix code removing it from TESTS variable. Actually, IOPR tests are not called from libpkix.sh, but from ocsp.sh (this script tests only IOPR). 
This patch should fixes the problem.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 286052 [details] [diff] [review]
Fix to all.sh

Bob, what's the reason of excluding IOPR tests from Shared DB testing ?
Attachment #286052 - Flags: review?(rrelyea)
There is no need to run libpkix unit tests more than once. It would be a wast of time to run these tests again in shared db test run.
I've created a new bug (bug 401610) about the IOPR issues in shared DB testing.
It turns out to be a real bug.
Slavo,

I've found a couple of failures that aren't showing up in tinderbox:

1) If I turn IOPR on I get failures in the PKIX tests.
2) If I turn PKIX on, I get failures in the update DB case.

I'll look into #2, but I suspect that the issue is environmental (though it could be some other issue we are tripping over.

bob
Depends on: 401610
Attachment #286052 - Flags: review?(rrelyea)
Fix for problem 2 from comment 28. After changing HOSTDIR from backup script there were not set all variables related to it. That caused dbupgrade was called for PKIX directories. This also caused failures in nightly builds.
Attachment #286671 - Flags: review?(rrelyea)
Comment on attachment 286671 [details] [diff] [review]
Environment backup fix.

r+ Excellent!
Attachment #286671 - Flags: review?(rrelyea) → review+
Status: REOPENED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.12
You need to log in before you can comment on or make changes to this bug.