[Twg] Separated test execution

Tue May 15 17:41:43 UTC 2012

On 2012-05-14, at 2:59 PM, Alexander Lezhoev wrote:
> By our estimation, there are about 30 tests need for the  improvement. If we settle this question, we can significantly improve test-framework automation potential.
> Please share your opinions about this question and help to make a decision about it.
> The questions are
> 	• Do we want to have an ability to run each test independently?
> 	• What is more acceptable - unite sequential tests into complex ones or supplement exists test with additional code steps?   

Thanks for looking at this.  I agree that it makes sense to be able to run subtests independently, and this has been the accepted policy for some time.  While there are some subtests that may not be in this state today, that there are only about 30 such subtests indicates that there isn't a huge problem that needs a complex solution to resolve.

I think the method to fix each subtest depends on the particular subtest under discussion.  If the subtest itself is already complex, and just has a common setup step, then they should be kept separate.

In the case shown below, these subtests were created back when Lustre could sometimes fail to run simple operations like cross-node create/unlink, but it probably makes sense to combine these subtests into a single subtest as in your example #1.  Since there is already typically an increasing amount of overhead to run each subtest, having a single "mkdir" for a subtest probably does not make sense anymore.

> Typical problem is sanityn test_1
> 
> test_1a() {
>    touch $DIR1/f1
>    [ -f $DIR2/f1 ] || error
> }
> run_test 1a "check create on 2 mtpt's =========================="
> 
> test_1b() {
>    chmod 777 $DIR2/f1
>    $CHECKSTAT -t file -p 0777 $DIR1/f1 || error
>    chmod a-x $DIR2/f1
> }
> run_test 1b "check attribute updates on 2 mtpt's ==============="
> 
> test_1c() {
>    $CHECKSTAT -t file -p 0666 $DIR1/f1 || error
> }
> run_test 1c "check after remount attribute updates on 2 mtpt's ="
> 
> test_1d() {
>    rm $DIR2/f1
>    $CHECKSTAT -a $DIR1/f1 || error
> }
> run_test 1d  "unlink on one mountpoint removes file on other ===="
> 
> 
> They cannot be run separately, because the next index uses the code of       previous one. This means all tests should be run in groups of letter indexes, or they should be refactored to run independently. 
> 
> Some of tests have been already refactored to run “letters” separately, but we have to make a rule which we should follow and use for further refactoring.
> There are three decisions we can take about this situation
> 	• Join the code of all test steps into single test with corresponding number. So we will have one test_1 instead of test_1a .. test_1d in the described case.
> 	• Move the code of steps to corresponding functions which will be called from each step. In other words the next indexed test will duplicate some functionality of previous one.
> 	• Do nothing and decide that “letters” mustn’t be executed independently, but only in “number” group.
> 
> The first variant could be implemented as follows.
> 
> test_1() {
>         touch $DIR1/f1
>         [ -f $DIR2/f1 ] || error "check create on 2 mtpt's failed"
>         chmod 777 $DIR2/f1
>         $CHECKSTAT -t file -p 0777 $DIR1/f1 ||
>                 error "check attribute updates on 2 mtpt's failed"
>         chmod a-x $DIR2/f1
>         $CHECKSTAT -t file -p 0666 $DIR1/f1 ||
>                 error "after remount attributes on 2 mtpt's failed"
>         rm $DIR2/f1
>         $CHECKSTAT -a $DIR1/f1 ||
>                 error "unlink on one mtpt removes file on other failed"
> }
> run_test 1 "check attributes updates on 2 mtpt's"

Note I've reformatted the above example to follow the proper coding style (80-column lines).  Also note that in this case, it appears there originally was supposed to be an unmount/remount between "chmod a-x" and the following "$CHECKSTAT -p 0666", but that was lost over time.

> This approach has disadvantage that such kind of refactoring will lead to reduction of test numbering and it will hard to work with regression history of the refactored tests.

I doubt there are (m)any cases where these subtests have failed, so I don't think there is any serious loss in testing history.  Also, we shouldn't be prevented from improving some of the testing code just because it will be a break from a few of the old subtest results.

While testing continuity is important (e.g. I wouldn't advocate renumbering subtests just to give them "nicer" numbers), if this can improve some of the subtests that are causing problems there is a clear benefit to do so.

> I’ve omitted functions code - their content is obvious.
> This disadvantage of this approach — summary increase of tests run-time (the next test duplicates code of previous one). But the necessity of all these tests is doubtful here, because the last one includes first three tests.

Right.  While it is true that virtually all later subtests will already do a "mkdir" internally, there is still a benefit to knowing early that the "mkdir" itself is working properly before spending time trying to debug a complex subtest only to find some simple operation caused the failure.

> Very similar situation is for recovery-small 1, 2, 3 and 4, 5.
> 
> test_1() {
> drop_request "mcreate $DIR/f1" || return 1
> drop_reint_reply "mcreate $DIR/f2" || return 2
> }
> run_test 1 "mcreate: drop req, drop rep"
> 
> test_2() {
> drop_request "tchmod 111 $DIR/f2"  || return 1
> drop_reint_reply "tchmod 666 $DIR/f2"  || return 2
> }
> run_test 2 "chmod: drop req, drop rep"
> 
> test_3() {
> drop_request "statone $DIR/f2" || return 1
> drop_reply "statone $DIR/f2"   || return 2
> }
> run_test 3 "stat: drop req, drop rep"
> 
> These three tests are actually steps of a single test scenario, because they work with the results of previous ones.
> 
> or join them into one and remove test_2 and test_3.
> 
> test_1() {
>    drop_request "mcreate $DIR/f1"  || return 1
>    drop_reint_reply "mcreate $DIR/f2"    || return 2
>    drop_request "tchmod 111 $DIR/f2"  || return 3
>    drop_reint_reply "tchmod 666 $DIR/f2"    || return 4
>    drop_request "statone $DIR/f2" || return 5
>    drop_reply "statone $DIR/f2"   || return 6
> }
> run_test 1 "mcreate, chmod,stat: drop req, drop,req"

As someone else commented, I really dislike the use of error codes to signal failures.  The "1", "2", ... return codes have no real meaning, and seeing a result like "recovery small test_1 failed with 3" is useless compared to "recovery_small test_1 failed when resending chmod request".

If you are doing any significant subtest restructuring, please use "error" instead of "return".

> Another big example are sanity tests 200 and 201. Here is the part of the resulting code after refactoring, so we can separately run each letter index:
> 
> test_200a() {
>   test_200_create_pool
>   test_201_remove_pool
> }
> run_test 200a "Create new pool =========================================="

If restructuring subtests, please also drop the trailing "===" markers.  These are handled by the test-framework now, and just make the output ugly.

> test_200b() {
>   test_200_create_pool
>   test_200_add_targets
>   test_201_remove_all_targets
>   test_201_remove_pool
> }
> 
> . . . 
> 
> test_201b() {
>   test_200_create_pool
>   test_200_add_targets
>   test_200_dir_set_pool
>   test_200_check_dir_pool
>   test_200_check_file_alloc
>   test_200_create_files
>   test_200_create_relative_path_files
>   test_201_remove_all_targets
>   test_201_remove_pool
> }
> run_test 201b "Remove all targets from a pool =========================="
> 
> test_201c() {
>   test_200_create_pool
>   test_201_remove_pool
> }
> run_test 201c "Remove a pool ============================================"
> 
> 
> We have to include cleanup steps here to make possible to run letter indexes independently. With that cleanup steps, 200a and 201c became absolutely equal and need to be reduced. Same situation is for 200h and 201b.

The problem with doing a full setup and cleanup for each subtest is that this can make the test runtime longer than necessary.  In previous cases where there was a dependency between subtests, it is better to have the "setup" step do a "check or setup", and if the previous subtest has already run, there is no additional work done for the setup.  Similarly, if every subtest does its own cleanup, but the next test sets everything back up again, it is just a waste of testing time.

I also don't think each subtest should do its own cleanup, unless the subtest itself could cause problems with other subtests because it significantly fills the OST(s) or the MDT(s).  The cleanup for "normal" tests should be deferred until after the end of all subtests (e.g. the "rm -rf $DIR/[df][0-9]*" for sanity.sh and sanityn.sh).  The pool tests themselves already have a "cleanup_pools" test that is run after all of the pools tests are completed.

Cheers, Andreas
--
Andreas Dilger                       Whamcloud, Inc.
Principal Lustre Engineer            http://www.whamcloud.com/