Hi Serg,

Thanks for the review. I've addressed the review comments in commit 1cedc95
in branch 10.2-MENT-566. See below for comments.

On Mon, Mar 30, 2020 at 7:34 PM Sergei Golubchik <serg@mariadb.org> wrote:

> 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@iki.fi>
> > committer: Rasmus Johansson <razze@iki.fi>
> > 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
>

Removed


> >
> >  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)
>

Done

>
> > +
> >  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
>

Removed in a couple of places.


> > +      $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 ..."
>

Changed to use that syntax.


> 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
>

Switched to using qq(...);


> > +        $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.
>

See next comment.

>
> > +    }
> > +
> > +    $xml_report = $xml_report . "</testsuites>\n";
>
> or just combine it with this one:
>
>     $xml_report .= "\t</testsuite>\n</testsuites>\n";
>

Done this way.


> > +
> > +    # 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)
>

added die and changed to 3-form open


> > +    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
>

Removed


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

BR,
Rasmus