maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxxx>]

Hi, Monty!

On May 04, Michael Widenius wrote:
> At lp:maria/5.5
> 
> ------------------------------------------------------------
> revno: 3393
> revision-id: monty@xxxxxxxxxxxx-20120504133709-w1l2t1vhvpqupu3v
> parent: monty@xxxxxxxxxxxx-20120503130041-fb9myku0uh1qybty
> committer: Michael Widenius <monty@xxxxxxxxxxxx>
> branch nick: maria-5.5
> timestamp: Fri 2012-05-04 16:37:09 +0300
> message:
>   Added --continue-on-error to mysqltest and mysql-test-run
>   This will contune the test case even if there was an error. This makes it easier to run a test that contains many sub tests against one engine.

First, you forgot to test the new feature.
see mysqltest.test for examples

> === modified file 'client/mysqltest.cc'
> --- a/client/mysqltest.cc	2012-02-23 06:50:11 +0000
> +++ b/client/mysqltest.cc	2012-05-04 13:37:09 +0000
> @@ -1746,9 +1791,15 @@ static int diff_check(const char *diff_n
>    my_snprintf(buf, sizeof(buf), "%s -v", diff_name);
>  
>    if (!(res_file= popen(buf, "r")))
> -    die("popen(\"%s\", \"r\") failed", buf);
> +  {
> +    report_or_die("popen(\"%s\", \"r\") failed", buf);
> +    return;
> +  }

I doubt it makes sense to continue in this case

> -  /* if diff is not present, nothing will be in stdout to increment have_diff */
> +  /*
> +    if diff is not present, nothing will be in stdout to increment
> +    have_diff
> +  */
>    if (fgets(buf, sizeof(buf), res_file))
>      have_diff= 1;
>  
> @@ -2069,9 +2120,13 @@ void check_result()
>  
>    switch (compare_files(log_file.file_name(), result_file_name)) {
>    case RESULT_OK:
> -    break; /* ok */
> +    if (!error_count)
> +      break; /* ok */
> +    mess= "Got errors while running test";

I wonder whether you need it. As the error goes into the result,
it'll inevitably end up in the .reject file.
one can get RESULT_OK only if the same error is present
in the .result file. But in this case you won't increment
error_count, because an error in the result file means --error directive.

> +    /* Fallthrough */
>    case RESULT_LENGTH_MISMATCH:
> -    mess= "Result length mismatch\n";
> +    if (!mess)
> +      mess= "Result length mismatch\n";
>      /* Fallthrough */
>    case RESULT_CONTENT_MISMATCH:
>    {
> @@ -3240,8 +3319,10 @@ void do_exec(struct st_command *command)
>        log_msg("exec of '%s' failed, error: %d, status: %d, errno: %d",
>                ds_cmd.str, error, status, errno);
>        dynstr_free(&ds_cmd);
> -      die("command \"%s\" failed\n\nOutput from before failure:\n%s\n",
> -          command->first_argument, ds_res.str);
> +      if (!opt_continue_on_error)
> +        die("command \"%s\" failed\n\nOutput from before failure:\n%s\n",
> +            command->first_argument, ds_res.str);

why not report_or_die ?

> +      return;
>      }
>  
>      DBUG_PRINT("info",
> @@ -3941,12 +4030,12 @@ void read_until_delimiter(DYNAMIC_STRING
>          No characters except \n are allowed on
>          the same line as the command
>        */
> -      die("Trailing characters found after command");
> +      report_or_die("Trailing characters found after command");

I think --continue-on-error should not affect syntax errors
in the test script itself. They should always abort the execution.

in general, you've documented the new option as

  "This is mostly useful when testing a storage engine and one wants to see
  how much of a multi-test file it can execute."

which is also what BigFish wanted it to do.
So, most (if not all) mysqltest failures not related to
a storage engine should not be ignored.

>      }
>  
>      if (feof(cur_file->file))
> -      die("End of file encountered before '%s' delimiter was found",
> -          ds_delimiter->str);
> +      report_or_die("End of file encountered before '%s' delimiter was found",
> +                    ds_delimiter->str);
>  
>      if (match_delimiter(c, ds_delimiter->str, ds_delimiter->length))
>      {
> === modified file 'mysql-test/mysql-test-run.pl'
> --- a/mysql-test/mysql-test-run.pl	2012-05-03 13:00:41 +0000
> +++ b/mysql-test/mysql-test-run.pl	2012-05-04 13:37:09 +0000
> @@ -5805,6 +5807,11 @@ sub start_mysqltest ($) {
>      mtr_add_arg($args, "--max-connections=%d", $opt_max_connections);
>    }
>  
> +  if ($opt_continue_on_error)
> +  {
> +    mtr_add_arg($args, "--continue-on-error");

I think this should enable --force too.
or, perhaps, --force should enable this and one does not need
a separate --continue-on-error in mtr.

> +  }
> +
>    if ( $opt_embedded_server )
>    {
> 

Regards,
Sergei