Refactoring a Script You Must Not Break

Part II · Module 27 · Chapter 27.6 · Refactoring a Script You Must Not Break

1. The Inherited Script

Refactoring means changing the structure of code without changing what it does. For a flow script, "what it does" has a precise meaning: everything a caller can observe — what it prints, what it reports on standard error, and its exit status. If any of those change, it is not a refactor any more, whatever the intention was.

Here is the script this chapter refactors. It is a specimen written for this chapter, in the shape inherited scripts really have: no strict, global variables everywhere, patterns written inline, and a summary that has been "working" for years.

#!/usr/bin/perl
# old_summary.pl -- summarise simulation logs.  (inherited; works; nobody owns it)
$total = 0;
foreach $f (@ARGV) {
    open(F, $f) || die "cannot open $f";
    $text = "";
    while (<F>) { $text .= $_; }
    close(F);
    $name = $f;
    $name =~ s/.*\///;
    $name =~ s/\.log$//;
    if ($text =~ /ERROR/ || $text =~ /FATAL/) { $status{$name} = "FAIL"; $nfail++; }
    elsif ($text =~ /finish/) { $status{$name} = "PASS"; $npass++; }
    else { $status{$name} = "UNKNOWN"; $nunk++; }
    if ($text =~ /^ERROR: (.*)$/m) { $first{$name} = $1; }
    $total++;
}
foreach $t (keys %status) {
    print "$t: $status{$t}";
    print "  first error: $first{$t}" if $first{$t};
    print "\n";
}
print "total $total, pass $npass, fail $nfail, unknown $nunknown\n";

Its inputs are the ten real logs from Chapters 27.1 and 27.4 — the six golden run endings and the four bad inputs:

#!/bin/bash
ls inputs | sed 's/^/  /'

  icarus_empty.log
  icarus_finish_1.log
  icarus_info_mentions_error.log
  icarus_killed.log
  icarus_mismatch.log
  icarus_pass.log
  icarus_pass_then_killed.log
  icarus_sv_error.log
  icarus_sv_fatal.log
  iverilog_compile_error.log

The goal is to restructure this script so that it can later use the shared module from Chapter 27.1. This chapter only changes its shape; Chapter 27.7 deals with its faults. Keeping those two jobs apart is most of the discipline.

2. Before a Baseline: Is the Output Even Repeatable?

The method is to capture what the script produces now, then compare after every change. That only works if the script produces the same output every time it is run on the same inputs. Check that first:

#!/bin/bash
for i in 1 2 3 4 5; do perl old_summary.pl inputs/*.log | cksum; done > runs.txt
if [ "$(sort -u runs.txt | wc -l)" -eq 1 ]; then
    echo "5 runs: identical every time"
else
    echo "5 runs: the output was NOT the same every time"
fi

5 runs: the output was NOT the same every time

The output changes from run to run. The last loop walks keys %status, and Perl returns a hash's keys in a different order in every process (Chapter 17.8 measured this and made the case for sorted output). The driver deliberately does not print how many different outputs it saw, or any of them: that number is itself random, and a page cannot publish a result that changes every time it is produced.

A baseline taken from this script would be one random ordering. Every comparison afterwards would report differences that are only the order, and real changes would be lost among them — or, worse, the comparison would be abandoned as "too noisy".

3. Make It Repeatable, and Prove Only the Order Changed

So the first change is not a refactor: it makes the output deterministic, by sorting the keys. It is the one behaviour change allowed before the baseline, and it gets its own check — that it changed only the order, not the content:

#!/bin/bash
# The one-word change: keys %status -> sort keys %status
perl -pe 's/^foreach \$t \(keys %status\)/foreach \$t (sort keys %status)/' \
    old_summary.pl > step1_sorted.pl
diff old_summary.pl step1_sorted.pl | sed 's/^/  /'

# Same lines as before, only reordered?
perl old_summary.pl  inputs/*.log | sort > before.sorted
perl step1_sorted.pl inputs/*.log | sort > after.sorted
cmp -s before.sorted after.sorted && echo "same lines as the original, in a fixed order"

for i in 1 2 3 4 5; do perl step1_sorted.pl inputs/*.log | cksum; done | sort -u | wc -l \
    | awk '{ print ($1 == 1 ? "5 runs: identical every time" : "still not repeatable") }'

  18c18
  < foreach $t (keys %status) {
  ---
  > foreach $t (sort keys %status) {
same lines as the original, in a fixed order
5 runs: identical every time

The check compares the two outputs after sorting both — so it ignores order and compares everything else. They are the same lines. Then five runs of the new version are identical. Now there is something to take a baseline of.

4. The Baseline Is Everything a Caller Can See

The canonical example for this chapter redirects standard output to baseline.txt. That captures one of the three things a caller depends on. A flow script also communicates through standard error — warnings, diagnostics, the message a wrapper logs — and through its exit status, which is what Chapter 26.1's wrapper and every CI job act on. A refactor that keeps stdout identical but starts printing a warning, or exits 2 where it exited 0, has changed the script's behaviour for its callers.

So the baseline is a directory holding all three:

#!/bin/bash
# snapshot.sh SCRIPT DIR -- record everything a caller can observe about a
# run of SCRIPT on the fixed inputs: stdout, stderr and the exit status.
set -u
script=$1 dir=$2
mkdir -p "$dir"
perl "$script" inputs/*.log > "$dir/stdout" 2> "$dir/stderr"
echo $? > "$dir/status"

#!/bin/bash
# compare.sh SCRIPT -- run SCRIPT the same way as the baseline and report
# whether anything a caller can observe has changed. Exit 0 only if nothing did.
set -u
script=$1
rm -rf after
bash snapshot.sh "$script" after
if diff -ru baseline after > change.diff; then
    echo "$script: IDENTICAL to the baseline (stdout, stderr, exit status)"
    exit 0
fi
echo "$script: CHANGED"
sed -e 's/^/    /' change.diff | grep -vE '^    (diff -ru|--- |\+\+\+ )'
exit 1

snapshot.sh records one run. compare.sh records another run of a candidate script the same way and diffs the two directories, reporting either IDENTICAL or exactly what changed — in any of the three files. Its own exit status is 0 only when nothing changed, so it can stop a script or a CI job.

5. One Small Step at a Time

The refactor proceeds in steps small enough that, if one changes the output, it is obvious which edit did it. Each step is compared against the baseline before the next begins.

Step 2 moves the classification into a named subroutine. Nothing else changes; the subroutine returns exactly the three words the inline code assigned.

#!/usr/bin/perl
# step 2: the classification moved into a named subroutine. Nothing else changed.
$total = 0;
foreach $f (@ARGV) {
    open(F, $f) || die "cannot open $f";
    $text = "";
    while (<F>) { $text .= $_; }
    close(F);
    $name = $f;
    $name =~ s/.*\///;
    $name =~ s/\.log$//;
    $status{$name} = classify($text);
    if    ($status{$name} eq "FAIL") { $nfail++ }
    elsif ($status{$name} eq "PASS") { $npass++ }
    else                             { $nunk++ }
    if ($text =~ /^ERROR: (.*)$/m) { $first{$name} = $1; }
    $total++;
}
foreach $t (sort keys %status) {
    print "$t: $status{$t}";
    print "  first error: $first{$t}" if $first{$t};
    print "\n";
}
print "total $total, pass $npass, fail $nfail, unknown $nunknown\n";

sub classify {
    my ($text) = @_;
    return "FAIL" if $text =~ /ERROR/ || $text =~ /FATAL/;
    return "PASS" if $text =~ /finish/;
    return "UNKNOWN";
}

Step 3 adds use strict and declares every variable in the smallest scope it needs (Chapter 12.7). The bareword filehandle F becomes a lexical $fh.

#!/usr/bin/perl
# step 3: use strict, and every variable declared in the smallest scope.
use strict;

my ($total, $npass, $nfail, $nunk) = (0);
my $nunknown;   # FOUND BY strict: counted as $nunk, printed as $nunknown.
                # Kept as-is in this step -- a refactor must not change output.
my (%status, %first);

foreach my $f (@ARGV) {
    open(my $fh, "<", $f) || die "cannot open $f";
    my $text = "";
    while (<$fh>) { $text .= $_; }
    close($fh);
    my $name = $f;
    $name =~ s/.*\///;
    $name =~ s/\.log$//;
    $status{$name} = classify($text);
    if    ($status{$name} eq "FAIL") { $nfail++ }
    elsif ($status{$name} eq "PASS") { $npass++ }
    else                             { $nunk++ }
    if ($text =~ /^ERROR: (.*)$/m) { $first{$name} = $1; }
    $total++;
}
foreach my $t (sort keys %status) {
    print "$t: $status{$t}";
    print "  first error: $first{$t}" if $first{$t};
    print "\n";
}
print "total $total, pass $npass, fail $nfail, unknown $nunknown\n";

sub classify {
    my ($text) = @_;
    return "FAIL" if $text =~ /ERROR/ || $text =~ /FATAL/;
    return "PASS" if $text =~ /finish/;
    return "UNKNOWN";
}

#!/bin/bash
perl -pe 's/^foreach \$t \(keys %status\)/foreach \$t (sort keys %status)/' \
    old_summary.pl > step1_sorted.pl
bash snapshot.sh step1_sorted.pl baseline
echo "baseline: $(wc -l < baseline/stdout | tr -d ' ') lines of stdout, $(wc -c < baseline/stderr | tr -d ' ') bytes of stderr, exit $(cat baseline/status)"
bash compare.sh step2_named_sub.pl
bash compare.sh step3_lexical.pl

baseline: 11 lines of stdout, 0 bytes of stderr, exit 0
step2_named_sub.pl: IDENTICAL to the baseline (stdout, stderr, exit status)
step3_lexical.pl: IDENTICAL to the baseline (stdout, stderr, exit status)

Both steps are identical to the baseline in all three respects. The script now has a named, testable classification function and no undeclared globals — and it provably does exactly what it did before.

6. A Bug Found While Refactoring Is Written Down, Not Fixed

Step 3 found something. Adding use strict made Perl refuse to compile until every variable was declared, and declaring them exposed a typo the original author never saw: the loop counts unknown runs in $nunk, and the summary prints $nunknown — a different variable that is never set. Look at the last line of the baseline: unknown is followed by nothing. Four of the ten logs are unknown, and the summary has silently printed a blank for years.

Step 3 declares both variables and keeps the bug, with a comment saying so. That looks wrong, and it is the right call. A refactoring step that also fixes a bug produces a diff that mixes two things — "the structure changed" and "the output changed on purpose" — and nobody reviewing it can tell whether the output change was the fix or an accident. Keep them apart:

• The refactoring step changes structure and is proven identical.
• The fix is its own change, whose whole content is the output difference, reviewed as such. Chapter 27.7 makes it.

7. The Step That Changed Output

Step 4 is the kind of tidy-up a refactor invites. The script contains two different patterns for an error line — /ERROR/ in the classification and /^ERROR: (.*)$/m for the first-error message — and keeping one definition of each pattern is good practice (Chapter 11.18; Chapter 27.7 makes it a rule). So step 4 defines it once:

#!/usr/bin/perl
# step 3: use strict, and every variable declared in the smallest scope.
use strict;

my ($total, $npass, $nfail, $nunk) = (0);
my $nunknown;   # FOUND BY strict: counted as $nunk, printed as $nunknown.
                # Kept as-is in this step -- a refactor must not change output.
my (%status, %first);
my $ERROR_LINE = qr/^ERROR: (.*)$/m;   # one definition of an error line

foreach my $f (@ARGV) {
    open(my $fh, "<", $f) || die "cannot open $f";
    my $text = "";
    while (<$fh>) { $text .= $_; }
    close($fh);
    my $name = $f;
    $name =~ s/.*\///;
    $name =~ s/\.log$//;
    $status{$name} = classify($text);
    if    ($status{$name} eq "FAIL") { $nfail++ }
    elsif ($status{$name} eq "PASS") { $npass++ }
    else                             { $nunk++ }
    if ($text =~ $ERROR_LINE) { $first{$name} = $1; }
    $total++;
}
foreach my $t (sort keys %status) {
    print "$t: $status{$t}";
    print "  first error: $first{$t}" if $first{$t};
    print "\n";
}
print "total $total, pass $npass, fail $nfail, unknown $nunknown\n";

sub classify {
    my ($text) = @_;
    return "FAIL" if $text =~ $ERROR_LINE || $text =~ /FATAL/;
    return "PASS" if $text =~ /finish/;
    return "UNKNOWN";
}

#!/bin/bash
perl -pe 's/^foreach \$t \(keys %status\)/foreach \$t (sort keys %status)/' \
    old_summary.pl > step1_sorted.pl
bash snapshot.sh step1_sorted.pl baseline
bash compare.sh step4_one_pattern.pl
echo "compare.sh exit status: $?"

step4_one_pattern.pl: CHANGED
    @@ -1,6 +1,6 @@
     icarus_empty: UNKNOWN
     icarus_finish_1: PASS
    -icarus_info_mentions_error: FAIL
    +icarus_info_mentions_error: PASS
     icarus_killed: UNKNOWN
     icarus_mismatch: FAIL  first error: data mismatch exp=5 got=4
     icarus_pass: PASS
    @@ -8,4 +8,4 @@
     icarus_sv_error: FAIL  first error: tb/sv_error.v:3: data mismatch exp=5 got=4
     icarus_sv_fatal: FAIL
     iverilog_compile_error: UNKNOWN
    -total 10, pass 2, fail 4, unknown 
    +total 10, pass 3, fail 3, unknown 
compare.sh exit status: 1

The comparison caught it. The two "error" patterns were never the same pattern. The classification's /ERROR/ matched the word anywhere — including in the INFO lines of icarus_info_mentions_error.log — while the first-error pattern required ERROR: at the start of a line. Using one definition for both changed that log's verdict from FAIL to PASS, and the summary's counts with it.

And here is the instructive part: the new output is more correct. Chapter 27.4 established that this log is a passing run whose INFO lines mention an expected error. So step 4 fixed a real bug. It is still reverted from the refactor, for the reason section 6 gave: it is a behaviour change, and it must be made deliberately, on its own, with its diff as the description of what changed. The baseline did its job — it did not judge whether the change was good, it made sure nobody could make it without noticing.

When a change to the output is wanted, the procedure is the same every time: make the change alone, run compare.sh, read the diff as the change's specification, then regenerate the baseline from the new version as a separate, deliberate act. From then on the new output is what every later step is compared against.

8. A Baseline Is Only as Strong as Its Inputs

Step 4 was caught by exactly one input: icarus_info_mentions_error.log, one of Chapter 27.4's bad inputs. What if the baseline had been taken with only the six golden logs — the inputs most people would think of first?

#!/bin/bash
perl -pe 's/^foreach \$t \(keys %status\)/foreach \$t (sort keys %status)/' \
    old_summary.pl > step1_sorted.pl
mkdir -p set_aside
mv inputs/icarus_empty.log inputs/icarus_info_mentions_error.log \
   inputs/icarus_pass_then_killed.log inputs/iverilog_compile_error.log set_aside/
echo "inputs now: $(ls inputs | wc -l | tr -d ' ') logs"
bash snapshot.sh step1_sorted.pl baseline
bash compare.sh step4_one_pattern.pl

inputs now: 6 logs
step4_one_pattern.pl: IDENTICAL to the baseline (stdout, stderr, exit status)

Identical. With six inputs, step 4's behaviour change is invisible, and it would have been kept as "a harmless tidy-up" — changing the verdict of every future run whose INFO lines mention an error, with nothing to show for it.

A baseline comparison proves that the output did not change for these inputs. It says nothing about inputs that are not in the set. So the inputs deserve as much care as the comparison: the golden logs from Chapter 27.2, the bad inputs from Chapter 27.4, and — before refactoring a script that runs in production — a sample of the real inputs it has actually been processing, since those are what its users depend on.

9. Exercises

1. Prove step 3's claim. Step 3 replaced the bareword filehandle F with a lexical $fh. Find an input or situation in which that change could alter behaviour — think about what happens to F if two copies of the loop ever ran at once — and explain why it did not here. Is "identical on these inputs" enough evidence for this particular step?

2. Make the fix properly. Take step 4's pattern change as a deliberate behaviour change: apply it alone to step 3, run compare.sh, and write the review description of the change from the diff alone. Then regenerate the baseline and continue refactoring from the new version.

3. A step that changes only stderr. Add use warnings to step 3 and run compare.sh. Does anything change? Construct an input — a real log, captured — on which use warnings would change stderr, and decide whether that counts as a behaviour change for this script's callers.

4. Exit status as behaviour. The original script dies on an unreadable input. Change step 3 so that it prints a message and exits 2 instead — a reasonable improvement. What does compare.sh report for the current inputs, and what input must the baseline contain for the change to be seen at all?

5. Your own baseline. Pick a flow script you maintain and capture a baseline of it from a week of its real inputs. Run it twice and check it is repeatable. Write down every source of non-repeatability you find — hash order, timestamps, absolute paths, process IDs — and how you removed each one before taking the baseline.

10. What To Remember

• A refactor changes structure, not behaviour — and behaviour is everything a caller observes: stdout, stderr, exit status.
• Check the output is repeatable before taking a baseline. Measured: the inherited script's output was not the same across five runs (hash order).
• Make it repeatable first, and prove only the order changed: compare the sorted outputs.
• Snapshot stdout, stderr and exit status, and compare all three after every step.
• Steps small enough that a difference points at one edit. Measured: named sub, strict and my — identical.
• A bug found while refactoring is written down, not fixed in the same step. Measured: strict exposed $nunk vs $nunknown, a blank count printed for years.
• A step that changes output is reverted, even when the new output is better. Measured: one pattern definition turned a FAIL into a correct PASS.
• Behaviour changes are separate, reviewed changes; then regenerate the baseline on purpose.
• A baseline is only as strong as its inputs. Measured: with only the six golden logs, the same change was IDENTICAL.
• Include bad inputs and real production inputs in the baseline set.

Chapter 27.7 finishes the job on this script: the small set of changes that make an inherited script safe to edit, applied without a rewrite — including, at last, fixing the typo that step 3 found.