cassandane: support multiple output formats per run #4970

elliefm · 2024-07-12T01:26:44Z

This adds support to Cassandane for producing multiple formats of output from a single run, as long as they won't be fighting over stdout. In practice, this means "tap", "pretty", and "prettier" still cannot be used together, but "xml" can be used alongside any of them. This will become more useful if we add more non-stdout formats in the future (perhaps "json"), but this PR doesn't do that.

There's a bunch of internal architectural changes here, but the main outwardly-visible one should be that you can now specify the -f <format> argument to testrunner.pl multiple times. The other is that the --rerun behaviour after test failures now works independently of output format(s).

Reviewers: Please pull this branch and run Cassandane in any of the ways you usually would. If you can test this in an FM inabox or build system setup, even better. I'm confident that I haven't broken any of my own use cases, but less confident about use cases I don't use myself. The diff probably makes the most sense when read commit-by-commit.

This disables all output formats except 'tap' for now. They'll be added back as the old RunnerFoos are converted to FormatFoos

rjbs · 2024-08-16T01:35:37Z

cassandane/Cassandane/Unit/RunnerXML.pm

-use vars qw($VERSION);
-# XXX should this inherit from our own Cassandane::Unit::Runner?
-use base qw(Test::Unit::Runner);
+use lib '.';


Why is this being added? Feels like a code smell. The contents of @INC should probably be correct by now, unless I don't know how this works — which is possible!

Cassandane expects to run out of its own source tree, which means none of its modules are installed to anywhere @INC looks, so all of Cassandane needs to use lib '.'; before the use lines for its own stuff. See d03d49b

RunnerXML didn't already have this line because it was an oddball that didn't use any Cassandane modules, but now it does so it does.

This (use lib '.') is the kind of thing I'd expect in the program that uses the modules, not the modules themselves. For example, testrunner.pl. Then you could delete it from many other places.

rjbs

Like many heroic acts, I'm glad you did this and sorry it was required. 🤪

This looks like a nice clean refactoring and addition. I still really dislike the underlying code, but we gotta make do with what we have sometimes! This cleaned it up a little, which is nice.

I have not yet attempted to run this, only read it. I will give it a run tomorrowish.

rjbs · 2024-08-16T01:45:16Z

cassandane/Cassandane/Unit/RunnerXML.pm


-    $generator ||= XML::Generator->new(escape => 'always', pretty => 2);
+    my $self = $class->SUPER::new(@args);


Yeah, wow, the guts of this class were pretty gross, huh? I don't love this, but it seems less bizarre to me than the previous code!

rjbs · 2024-08-16T02:05:30Z

cassandane/Cassandane/Unit/Runner.pm

@@ -40,26 +40,28 @@
 package Cassandane::Unit::Runner;
 use strict;
 use warnings;
-use base qw(Test::Unit::TestRunner);
+use base qw(Test::Unit::Runner);


This change is kind of weird to me. This class is called Runner, but used to inherit from TestRunner… which itself inherits from Runner. The docs are not great here.

I think the deal is that a TestRunner is a more concrete kind of runner, and it used to be that our (Cass's) Runner was just specializing that. Now that we're building in this multiple formatter jobby, we're shifing our parent one level up toward the abstract. Fine!

Actually, even better, because we're no longer inhering from something and then changing the signature on its constructor, so I'll sleep easier!

Oh yeah this whole thing is a mess, I've slightly vacuumed one corner of one rug at best. And yeah your interpretation of what's going on here is correct, Cass's Runner has become more abstract. Specialising TestRunner used to get us output formatting (which we then mostly overrode anyway). Now our FormatFoo's take care of output formatting, and Runner can just be a Runner (albeit one that needs to know about FormatFoos, because the T:U:Runner doesn't)

rjbs · 2024-08-16T02:08:04Z

cassandane/testrunner.pl

-        usage unless defined $runners{$format};
+        my $format = $1;
+        usage unless defined $formatters{$format};
+        $want_formats{$format} = 1;


I would love to overhaul this code... but your changes are clearly improvements. They're just building on a mess. :)

rjbs · 2024-08-16T02:08:25Z

cassandane/testrunner.pl

-    open my $fh, '>&', \*STDOUT
-        or die "Cannot save STDOUT as a runner print stream: $!";
-    exit(! $runners{$format}->($plan, $fh));
+    $want_formats{tap} = 1 if not scalar keys %want_formats;


You don't need that scalar, it's implied by not.

Yeah, I try to be explicit in my intent because it shuts up the "wait, is that a bug???" voice and subsequent rabbit hole dive next time I look at the code after having forgotten writing it -- I'm not in perl often or deeply enough to keep the context rules and exceptions swapped in at all times. Also theoretically* communicates to the next reader that I considered this and made a deliberate choice, I wasn't merely fumbling pieces together until they worked. ;)

[* It could also communicate the exact opposite, depending what assumptions the reader starts from. Heh /sigh]

My advice is "never use scalar". It's usually confusing for an everyday Perl programmer. "Why was this needed? What thing here is special?" Generally you only need it when you do this:

%hash = ( key => EXPR, xyz => $foo, );

…and EXPR might evaluate to more than one thing. But that you could address by assigning EXPR to a scalar, and using it. Anyway, this isn't a big deal, but generally "using scalar will make a Perl code reviewer more confused than not". :)

rjbs · 2024-08-16T02:09:07Z

cassandane/testrunner.pl

+    my @writes_to_stdout = grep {
+        $formatters{$_}->{writes_to_stdout}
+    } keys %want_formats;
+    if (scalar @writes_to_stdout > 1) {


Don't need that scalar either, implied by > operator.

rjbs · 2024-08-16T02:15:04Z

cassandane/testrunner.pl

@@ -385,7 +371,7 @@ sub usage
    $plan->schedule(@names);

    # Run the schedule
-    $want_formats{tap} = 1 if not scalar keys %want_formats;
+    $want_formats{prettier} = 1 if not scalar keys %want_formats;


elliefm added 6 commits July 12, 2024 10:00

RunnerXML: use our base class, not generic

a8692fa

Runner: decouple output formatting from running tests

e6a702b

This disables all output formats except 'tap' for now. They'll be added back as the old RunnerFoos are converted to FormatFoos

Runner: add tell_formatters() for custom event handling

9bf013d

TestPlan: use tell_formatters() for fake_start_time event

dc91239

RunnerPretty: convert to FormatPretty

6c8cad8

RunnerXML: convert to FormatXML

ad97843

elliefm requested review from brong, rjbs, wolfsage, rsto, ksmurchison and lerlacher-fm July 12, 2024 01:26

lerlacher-fm added the le-tmp-mint-tag label Jul 12, 2024

rjbs reviewed Aug 16, 2024

View reviewed changes

rjbs approved these changes Aug 16, 2024

View reviewed changes

rsto removed their request for review August 20, 2024 06:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cassandane: support multiple output formats per run #4970

cassandane: support multiple output formats per run #4970

elliefm commented Jul 12, 2024

rjbs Aug 16, 2024 •

edited

Loading

elliefm Aug 16, 2024

rjbs Aug 16, 2024

rjbs left a comment

rjbs Aug 16, 2024

rjbs Aug 16, 2024

elliefm Aug 16, 2024

rjbs Aug 16, 2024

rjbs Aug 16, 2024

elliefm Aug 16, 2024

rjbs Aug 16, 2024

rjbs Aug 16, 2024

rjbs Aug 16, 2024


		$generator \|\|= XML::Generator->new(escape => 'always', pretty => 2);
		my $self = $class->SUPER::new(@args);

cassandane: support multiple output formats per run #4970

Are you sure you want to change the base?

cassandane: support multiple output formats per run #4970

Conversation

elliefm commented Jul 12, 2024

rjbs Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjbs Aug 16, 2024 •

edited

Loading