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