maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Rasmus!

Please, see below

On Mar 30, Rasmus Johansson wrote:
> revision-id: 46c66698d63 (mariadb-10.2.31-189-g46c66698d63)
> parent(s): 065a3d3eed7
> author: Rasmus Johansson <razze@xxxxxx>
> committer: Rasmus Johansson <razze@xxxxxx>
> timestamp: 2020-03-26 10:50:06 +0000
> message:
> 
> MENT-566 jUnit xml fixes
>
> diff --git a/mysql-test/lib/mtr_report.pm b/mysql-test/lib/mtr_report.pm
> index 3701ad79b15..1c6825d8fb7 100644
> --- a/mysql-test/lib/mtr_report.pm
> +++ b/mysql-test/lib/mtr_report.pm
> @@ -35,6 +37,7 @@ use My::Platform;
>  use POSIX qw[ _exit ];
>  use IO::Handle qw[ flush ];
>  use mtr_results;
> +use Cwd 'abs_path';

this seems to be unused now

>  
>  use Term::ANSIColor;
>  
> @@ -60,6 +63,8 @@ our $verbose;
>  our $verbose_restart= 0;
>  our $timer= 1;
>  
> +my $xml_report = "";

this could be moved down inside if ($::opt_xml_report)

> +
>  sub report_option {
>    my ($opt, $value)= @_;
>  
> @@ -402,6 +409,92 @@ sub mtr_report_stats ($$$$) {
>      print "All $tot_tests tests were successful.\n\n";
>    }
>  
> +  if ($::opt_xml_report) {
> +    my @sorted_tests = sort {$a->{'name'} cmp $b->{'name'}} @$tests;
> +    my $last_suite = "";
> +    my $current_suite = "";
> +    my $timest = isotime(time);
> +    my %suite_totals;
> +    my %suite_time;
> +    my %suite_tests;
> +    my %suite_failed;
> +    my %suite_disabled;
> +    my %suite_skipped;
> +    my $host = hostname;
> +    my $suiteNo = 0;
> +
> +    # loop through test results to count totals
> +    foreach my $test ( @sorted_tests ) {
> +      $current_suite = $test->{'suite'}->{'name'};
> +
> +      if ($test->{'timer'} eq "") {
> +        $test->{'timer'} = 0;
> +      }
> +
> +      # $suite_totals{$current_suite . "_time"} = $suite_totals{$current_suite . "_time"} + $test->{'timer'};
> +      # $suite_totals{$current_suite . "_tests"} = $suite_totals{$current_suite . "_tests"} + 1;

don't forget to remove all these commented-out lines before pushing

> +      $suite_time{$current_suite} = $suite_time{$current_suite} + $test->{'timer'};
> +      $suite_tests{$current_suite} = $suite_tests{$current_suite} + 1;
> +
> +      if ($test->{'result'} eq "MTR_RES_FAILED") {
> +        # $suite_totals{$current_suite . "_failed"} = $suite_totals{$current_suite . "_failed"} + 1;
> +        $suite_failed{$current_suite} = $suite_failed{$current_suite} + 1;
> +      } elsif ($test->{'result'} eq "MTR_RES_SKIPPED" && $test->{'disable'}) {
> +        # $suite_totals{$current_suite . "_disabled"} = $suite_totals{$current_suite . "_disabled"} + 1;
> +        $suite_disabled{$current_suite} = $suite_disabled{$current_suite} + 1;
> +      } elsif ($test->{'result'} eq "MTR_RES_SKIPPED") {
> +        $suite_skipped{$current_suite} = $suite_skipped{$current_suite} + 1;
> +      }
> +
> +      $suite_totals{"all_time"} = $suite_totals{"all_time"} + $test->{'timer'};
> +    }
> +
> +    # generate xml
> +    $xml_report = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n";
> +    $xml_report = $xml_report . "<testsuites disabled=\"" . $tot_disabled . "\" errors=\"\" failures=\"" . $tot_failed . "\" name=\"\" tests=\"" . $tot_tests . "\" time=\"" . $suite_totals{"all_time"} . "\">\n";

I find it more concise to write

    $xml_report .= "<testsuites ..."

instead of

    $xml_report = $xml_report . "<testsuites ..."

and also it's easier to read, because the first form directly means
"append to the variable", while the second means "concatenate two strings
and store the result" and by using the target variable also as first string
one gets the append effect.

> +
> +    foreach my $test ( @sorted_tests ) {
> +      $current_suite = $test->{'suite'}->{'name'};
> +
> +      if ($current_suite ne $last_suite) {
> +        if ($last_suite ne "") {
> +          $xml_report = $xml_report . "\t</testsuite>\n";
> +          $suiteNo++;
> +        }
> +
> +        $xml_report = $xml_report . "\t<testsuite disabled=\"" . $suite_disabled{$current_suite} . "\" errors=\"\" failures=\"" . $suite_failed{$current_suite} . "\" hostname=\"" . $host . "\" id=\"" . $suiteNo . "\" name=\"" . $current_suite . "\" package=\"\" skipped=\"" . $suite_skipped{$current_suite} . "\" tests=\"" . $suite_tests{$current_suite} . "\" time=\"" . $suite_time{$current_suite} . "\" timestamp=\"" . $timest . "\">\n";

why do you always avoid variable interapolation? The above line could've been written as

       $xml_report .= qq(\t<testsuite disabled="$suite_disabled{$current_suite}" errors="" failures="$suite_failed{$current_suite}" hostname="$host" id="$suiteNo" name="$current_suite" package="" skipped="$suite_skipped{$current_suite}" tests="$suite_tests{$current_suite}" time="$suite_time{$current_suite}" timestamp="$timest">\n);

which is notably shorter and more readable

> +        $last_suite = $current_suite;
> +      }
> +
> +      $xml_report = $xml_report . "\t\t<testcase assertions=\"\" classname=\"" . $current_suite . "\" name=\"$test->{'name'}\" status=\"$test->{'result'}\" time=\"" . $test->{timer} . "\"";
> +
> +      if ($test->{'result'} eq "MTR_RES_FAILED") {
> +        $xml_report = $xml_report . ">\n\t\t\t<failure message=\"\" type=\"" . $test->{'result'} . "\">\n<![CDATA[" . $test->{'logfile'} . "]]>\n\t\t\t</failure>\n\t\t</testcase>\n";
> +      } elsif ($test->{'result'} eq "MTR_RES_SKIPPED" && $test->{'disable'}) {
> +        $xml_report = $xml_report . ">\n\t\t\t<failure message=\"disabled\" type=\"" . $test->{'result'} . "\"/>\n\t\t</testcase>\n";
> +      } elsif ($test->{'result'} eq "MTR_RES_SKIPPED") {
> +        $xml_report = $xml_report . ">\n\t\t\t<failure message=\"skipped\" type=\"" . $test->{'result'} . "\"/>\n\t\t</testcase>\n";
> +      } else {
> +        $xml_report = $xml_report . " />\n";
> +      }
> +
> +      # check if last test
> +      if ($test eq @sorted_tests[$#sorted_tests]) {
> +        $xml_report = $xml_report . "\t</testsuite>\n";
> +      }

better to print this after the loop, then to do a check for every test.

it will not be wrong, because your condition

   $test eq @sorted_tests[$#sorted_tests]

can only be true once, for the very last test. So you can as well remove the whole if()
and put the unconditional

    $xml_report .= "\t</testsuite>\n";

after the loop.

> +    }
> +
> +    $xml_report = $xml_report . "</testsuites>\n";

or just combine it with this one:

    $xml_report .= "\t</testsuite>\n</testsuites>\n";

> +
> +    # save to file
> +    # my $xml_file = $::opt_vardir . "/log/" . $::opt_xml_report;
> +    my $xml_file = $::opt_xml_report;
> +
> +    open XML_FILE, ">" . $xml_file;

error checking? Even if it's just

   open XML_FILE, ">" , $xml_file or die "$!";

and always use the 3-form open(). Where ">" is a separate argument, not
concatenated to the file name (see how I wrote it above)

> +    print XML_FILE $xml_report;
> +    close XML_FILE;
> +  }
> +
>    if (@$extra_warnings)
>    {
>      print <<MSG;
> diff --git a/mysql-test/mysql-test-run.pl b/mysql-test/mysql-test-run.pl
> index ef37cb4144d..298ba0015a3 100755
> --- a/mysql-test/mysql-test-run.pl
> +++ b/mysql-test/mysql-test-run.pl
> @@ -80,6 +80,7 @@ use lib "lib";
>  
>  use Cwd ;
>  use Cwd 'realpath';
> +use Cwd 'abs_path';

this seems to be unused now

>  use Getopt::Long;
>  use My::File::Path; # Patched version of File::Path
>  use File::Basename;

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx