1 Apr
2020
1 Apr
'20
10:06 a.m.
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