Pre-Merge Review: Automation Discipline

Part II · Module 27 · Chapter 27.11 · Pre-Merge Review: Automation Discipline

1. A Tool Is Judged by Its Callers

The three previous reviews read the code. This one does not. A flow tool's callers — Chapter 26.1's wrapper, a Makefile, a scheduler, a CI job — never see its source. They see what happens when they run it, and they rely on five properties without checking them:

Each property has been measured somewhere in this course. This review checks all five at once, before a tool is merged, by running it.

2. The Tool as Submitted

The tool reads a directory of simulation logs, gives each one a verdict using the shared module from Chapter 27.1, prints the verdicts, writes them to a results file, and leaves a small summary for a dashboard. It works: run once, on a fresh checkout, it produces the right verdicts.

#!/usr/bin/perl
# regress_report_submitted.pl LOGDIR -- as submitted for review.
# Writes a verdict per log, and a summary for the dashboard.
use strict;
use warnings;
use FindBin qw($Bin);
use lib "$Bin/lib";
use VLSI::Report qw(parse_log);

my $dir = shift or die "usage: regress_report_submitted.pl LOGDIR\n";
mkdir "out" or die "cannot create out/: $!\n";

my %verdict;
for my $log (glob "$dir/*.log") {
    print "processing $log\n";
    (my $name = $log) =~ s{.*/|\.log$}{}g;
    $verdict{$name} = parse_log($log)->{verdict};
}

open my $all, ">>", "out/results.txt" or die "out/results.txt: $!\n";
for my $name (keys %verdict) {
    print "$name $verdict{$name}\n";
    print {$all} "$name $verdict{$name}\n";
}
close $all;

open my $dash, ">", "$Bin/../latest_summary.txt" or die "summary: $!\n";
print {$dash} scalar(keys %verdict), " tests\n";
close $dash;

Read it for the five properties before reading the review's result. Each one has at least one line that breaks it.

3. The Review as a Black Box, in a Sandbox

The review runs the tool, never reads it, and checks each property by observing what the tool does. Every run happens in a sandbox: a fresh directory ws/ containing the tool's own working directory ws/run, plus copies of the logs. Anything that appears under ws/ outside ws/run is a write outside the tool's directory — and because the sandbox is inside the review's own temporary directory, a tool that misbehaves damages nothing real.

#!/usr/bin/perl
# automation_review.pl TOOL -- check a tool the way its callers depend on it,
# from outside. Every run happens in a fresh sandbox: ws/run is the tool's own
# directory; anything else that changes under ws/ is a write outside it.
use strict;
use warnings;
use File::Path qw(make_path remove_tree);
use File::Copy qw(copy);
use File::Find qw(find);
use Digest::MD5 qw(md5_hex);
use Cwd qw(getcwd);

my $tool = shift or die "usage: automation_review.pl TOOL\n";
my $home = getcwd();

sub sandbox {                       # fresh ws/run with the tool, lib/ and logs
    remove_tree("ws");
    make_path("ws/run/lib/VLSI", "ws/logs_all", "ws/logs_pass");
    copy("$tool", "ws/run/tool.pl") or die "copy $tool: $!\n";
    copy("lib/VLSI/Report.pm", "ws/run/lib/VLSI/Report.pm") or die;
    copy($_, "ws/logs_all/") for glob "inputs/*.log";
    copy("inputs/$_.log", "ws/logs_pass/") for qw(icarus_pass icarus_info_mentions_error);
}
sub tree {                          # path => md5 of every file under ws/
    my %t;
    find({ no_chdir => 1, wanted => sub {
        return unless -f $_;
        open my $f, "<", $_ or return; binmode $f;
        $t{$_} = md5_hex(do { local $/; <$f> });
    } }, "ws");
    return \%t;
}
sub run {                           # run the tool in ws/run; (stdout, stderr, status)
    my ($logs) = @_;
    chdir "ws/run" or die;
    system("perl tool.pl ../$logs > ../stdout.txt 2> ../stderr.txt");
    my $rc = $? >> 8;
    chdir $home or die;
    my $read = sub { open my $f, "<", $_[0] or return ""; local $/; <$f> };
    my @r = (scalar $read->("ws/stdout.txt"), scalar $read->("ws/stderr.txt"), $rc);
    unlink "ws/stdout.txt", "ws/stderr.txt";
    return @r;
}

my %result;

# 1. Re-runnable: a second run in the same place succeeds and changes nothing.
sandbox();
my ($o1, undef, $r1) = run("logs_all");  my $t1 = tree();
my ($o2, $e2,  $r2) = run("logs_all");  my $t2 = tree();
my @changed = grep { ($t1->{$_} // "") ne ($t2->{$_} // "") } sort keys %{{ %$t1, %$t2 }};
$result{"1 re-runnable"} = ($r1 == $r2 && $o1 eq $o2 && !@changed)
    ? "yes" : "NO: second run exit $r2" . (@changed ? ", changed @changed" : "")
            . ($e2 =~ /(\S.*)/ ? " ($1)" : "");

# 2. Exit status matches the verdict: 0 when everything passed, non-zero otherwise.
#    A fresh sandbox for EACH run, so neither run can fail because of the other.
sandbox(); my (undef, undef, $rc_pass) = run("logs_pass");
sandbox(); my (undef, undef, $rc_all)  = run("logs_all");
$result{"2 exit matches verdict"} = ($rc_pass == 0 && $rc_all != 0)
    ? "yes" : "NO: all-pass exit $rc_pass, with-failures exit $rc_all";

# 3. Stdout is data only: every line is "NAME VERDICT".
sandbox();
my ($out) = run("logs_all");
my @noise = grep { !/^\w+ (?:PASS|FAIL|NO_FINISH|EMPTY_LOG|NO_VERDICT)$/ } split /\n/, $out;
$result{"3 data on stdout only"} = @noise ? "NO: " . scalar(@noise) . " non-data line(s), e.g. '$noise[0]'" : "yes";

# 4. Output order is fixed: five runs, one stdout.
my %seen;
for (1 .. 5) { sandbox(); my ($o) = run("logs_all"); $seen{$o}++ }
$result{"4 order fixed"} = keys %seen == 1 ? "yes" : "NO: 5 runs were not identical";

# 5. Writes only inside its own directory.
sandbox();
my $before = tree();
run("logs_all");
my $after = tree();
my @outside = grep { !m{^ws/run/} && !exists $before->{$_} } sort keys %$after;
$result{"5 writes only its own dir"} = @outside ? "NO: wrote @outside" : "yes";

remove_tree("ws");
printf "  %-26s %s\n", $_, $result{$_} for sort keys %result;

Each check, in plain terms:

1. Re-runnable — run twice in the same sandbox; the exit statuses, the standard output and every file under ws/ must be identical after both runs.
2. Exit matches verdict — run on logs that all pass, then on all ten (which include failures), each in a fresh sandbox; the first must exit 0 and the second must not.
3. Data on stdout only — every standard output line must be NAME VERDICT.
4. Order fixed — five separate runs must produce one identical standard output.
5. Own directory only — any file created under ws/ but outside ws/run is reported.

4. The Result

#!/bin/bash
echo "regress_report_submitted.pl:"
perl automation_review.pl regress_report_submitted.pl

regress_report_submitted.pl:
  1 re-runnable              NO: second run exit 17 (cannot create out/: File exists)
  2 exit matches verdict     NO: all-pass exit 0, with-failures exit 0
  3 data on stdout only      NO: 10 non-data line(s), e.g. 'processing ../logs_all/icarus_empty.log'
  4 order fixed              NO: 5 runs were not identical
  5 writes only its own dir  NO: wrote ws/latest_summary.txt

The submitted tool fails all five. Each failure is a specific line, and each one is the kind of thing that works perfectly in the author's single test run.

5. Property 1: Run It Twice

The second run exited 17: cannot create out/: File exists. The tool calls mkdir "out" or die, which is correct exactly once. A tool that cannot run twice cannot be retried after a crash, re-run after a fix, or run by a scheduler that re-runs failed jobs automatically. And the status, 17, is the system's EEXIST error number, because die exits with $! (Chapter 15.4) — a number no caller will recognise.

The run also hides a second re-run defect that the first one masked: the results file is opened with >>, append. Even with the directory problem fixed, every run would add another full set of verdicts to out/results.txt, and a dashboard reading it would count each test once per run.

The fix is two lines: make_path("out") from File::Path, which does nothing if the directory already exists (Chapter 17.5's idempotent run directories), and writing the results file whole each time, through a temporary file and a rename (Chapter 13.13).

6. Property 2: The Exit Status Must Match the Verdict

The submitted tool exited 0 on the logs that all pass and 0 on the logs with failures. It never sets an exit status, so it ends with 0 whatever it found. Chapter 26.1 measured what that does to a flow: every layer above the tool reads only the number, and a regression with four failures is green.

The fix is a documented contract — 0 every log passed, 1 any other verdict, 2 a usage error or unreadable input — and an explicit exit that implements it.

7. Property 3: Data and Diagnostics on Separate Streams

Ten of the submitted tool's standard output lines are not data: processing ../logs_all/icarus_empty.log and nine more like it, interleaved with the verdicts. A person running the tool at a terminal sees a helpful progress trace. A program reading its standard output — Chapter 26.3's consumer, a while read loop in a wrapper, a CSV import — sees ten lines it cannot parse, or worse, parses them.

The fix is to print progress to standard error. The person at the terminal still sees it; the program reading standard output never does. Chapter 15.12 set the rule — data on stdout, everything else on stderr — and this check is how a review enforces it.

8. Property 4: The Same Order Every Time

Five runs of the submitted tool did not produce identical output. It prints its verdicts in keys %verdict order, which Perl changes between runs — the same defect Chapter 27.6 had to remove before it could take a baseline. Every diff between two nights' results is now full of differences that are only the order, and the real change, when there is one, is hidden among them.

The fix is sort — on the file list it reads and on the keys it prints (Chapter 17.8).

9. Property 5: Write Only Inside Your Own Directory

The submitted tool wrote ws/latest_summary.txt — outside its run directory. The line is "$Bin/../latest_summary.txt": one directory above wherever the script happens to be installed. In a real flow that is a shared directory, and two regressions running at once overwrite each other's summary; a cleanup step that deletes "everything in the run directory" leaves it behind, stale, for the dashboard to show the next day.

A tool writes only inside the directory it was given or is running in. If something outside needs the summary, the caller copies it there — Chapter 26.3's hand-off, made explicit.

Here is the tool with all five fixed:

#!/usr/bin/perl
# regress_report.pl -- one verdict per simulation log, for people and programs.
#   usage:  regress_report.pl LOGDIR
#   output: stdout "NAME VERDICT" per log, sorted; out/results.txt, the same
#           lines, rewritten whole on every run. Progress goes to stderr.
#   exit:   0 every log PASS; 1 any other verdict; 2 usage or unreadable input
use strict;
use warnings;
use FindBin qw($Bin);
use lib "$Bin/lib";
use File::Path qw(make_path);
use File::Temp qw(tempfile);
use VLSI::Report qw(parse_log);

my $dir = shift;
unless (defined $dir && -d $dir) {
    print STDERR "usage: regress_report.pl LOGDIR\n";
    exit 2;
}
make_path("out");                                     # fine if it already exists

my %verdict;
for my $log (sort glob "$dir/*.log") {
    print STDERR "processing $log\n";                 # progress: a diagnostic
    (my $name = $log) =~ s{.*/|\.log$}{}g;
    $verdict{$name} = parse_log($log)->{verdict};
}

my ($fh, $tmp) = tempfile("results.XXXXXX", DIR => "out");
for my $name (sort keys %verdict) {
    print "$name $verdict{$name}\n";
    print {$fh} "$name $verdict{$name}\n";
}
close $fh or die "cannot finish $tmp: $!\n";
rename $tmp, "out/results.txt" or die "cannot rename $tmp: $!\n";

exit((grep { $_ ne "PASS" } values %verdict) ? 1 : 0);

#!/bin/bash
echo "regress_report.pl:"
perl automation_review.pl regress_report.pl

regress_report.pl:
  1 re-runnable              yes
  2 exit matches verdict     yes
  3 data on stdout only      yes
  4 order fixed              yes
  5 writes only its own dir  yes

All five pass. The fixed tool is a few lines longer, and every added line answers one property: make_path, the temporary file and rename, sort in two places, progress on STDERR, the usage check, and the explicit exit.

10. The Review's Own Bug

The first version of automation_review.pl reported that the submitted tool passed the exit-status check. It ran both runs for check 2 in the same sandbox. The first run, on the passing logs, exited 0 and created out/. The second run then died on mkdir with exit 17 — non-zero — so "exits non-zero when there are failures" appeared to hold. The tool had not reported the failures at all; it had crashed on an unrelated defect, and the crash looked like the right answer.

The fix was the two-line comment now in the script: a fresh sandbox for each run, so neither run can fail because of the other. With it, the check reports what the tool really does — exit 0 both times.

That bug is the whole of this module in miniature. A check that passes is evidence only if it passes for the reason it claims, and the way to know is to make sure it can fail on the thing it tests (Chapter 27.4) and nothing else. Review tools are code. They need the same scrutiny as the tools they review.

11. Exercises

1. One defect at a time. The submitted tool fails all five checks at once, which does not prove that each check works on its own. Write five variants of the fixed tool, each with exactly one property broken, and confirm the review flags exactly one property for each. Which check was hardest to break in isolation?

2. The hidden append. Fix only the mkdir line in the submitted tool and re-run the review. What does check 1 now report about out/results.txt, and why was that defect invisible until the first one was fixed?

3. A sixth property. Add a check that a failing run leaves no partial output behind — for example, that out/results.txt is either the previous complete file or the new complete file after the tool is killed half-way. How would you kill it at a repeatable point?

4. Into the entry point. Combine this review with Chapter 27.3's prove and Chapter 27.10's maint_review.pl into one pre-merge script for a flow repository. In what order should they run, and what should the combined script's own exit status be?

5. Review a real tool. Run automation_review.pl's five checks — adapted to its arguments — on a tool from a flow you use. Report which properties fail, and for each, the concrete flow failure it would cause.

12. What To Remember

• Callers judge a tool by running it, so review it the same way — as a black box, in a sandbox.
• Five properties: re-runnable, exit matches verdict, data on stdout only, fixed order, writes only its own directory.
• Measured: the submitted tool failed all five; the fixed tool passed all five.
• Re-runnable: mkdir ... or die failed the second run with exit 17 (EEXIST, 15.4); >> would double the results. Use make_path and write files whole (13.13, 17.5).
• Exit matches verdict: the tool exited 0 with and without failures. Document the contract and exit explicitly (26.1).
• Data on stdout only: 10 progress lines polluted the data. Progress goes to stderr (15.12).
• Fixed order: five runs, not identical. sort the inputs and the outputs (17.8).
• Own directory only: $Bin/../latest_summary.txt wrote outside the run. The caller copies results out (26.3).
• A check that passes for the wrong reason is worse than none. Measured: the first review passed the exit check because a crash looked like a failure. Give every run its own sandbox.

That closes Module 27. You can now take a working parser and make it infrastructure a team can rely on. You can give three divergent scripts one shared module with a written contract each. You can test it against fixtures captured from the real tool rather than typed from memory, run the suite with Test::More and prove, and prove it can fail with negative tests and planted bugs. You can keep a table of real lines — including the ones that must not match — for every pattern and tool version. You can restructure an inherited script one proven step at a time, and make it safe to edit without rewriting it. And you can review every change with four checklists, each backed by a measurement: for patterns, for parsers, for maintainability, and for the behaviour every caller of a flow tool relies on.