From 64a3e75c10c868713f4ec512c4475e9e028b0df9 Mon Sep 17 00:00:00 2001
From: Graham Christensen <graham@grahamc.com>
Date: Mon, 5 Apr 2021 19:29:45 +0000
Subject: [PATCH 1/7] Event: init structure and parse existing messages

---
 src/lib/Hydra/Event.pm               |  30 +++++++
 src/lib/Hydra/Event/BuildFinished.pm |  25 ++++++
 src/lib/Hydra/Event/BuildStarted.pm  |  25 ++++++
 src/lib/Hydra/Event/StepFinished.pm  |  29 +++++++
 t/Event.t                            | 113 +++++++++++++++++++++++++++
 5 files changed, 222 insertions(+)
 create mode 100644 src/lib/Hydra/Event.pm
 create mode 100644 src/lib/Hydra/Event/BuildFinished.pm
 create mode 100644 src/lib/Hydra/Event/BuildStarted.pm
 create mode 100644 src/lib/Hydra/Event/StepFinished.pm
 create mode 100644 t/Event.t

diff --git a/src/lib/Hydra/Event.pm b/src/lib/Hydra/Event.pm
new file mode 100644
index 00000000..ac1222e0
--- /dev/null
+++ b/src/lib/Hydra/Event.pm
@@ -0,0 +1,30 @@
+package Hydra::Event;
+
+
+use strict;
+use Hydra::Event::BuildFinished;
+use Hydra::Event::BuildStarted;
+use Hydra::Event::StepFinished;
+our @ISA = qw(Exporter);
+our @EXPORT = qw(
+    parse_payload
+);
+
+my %channels_to_events = (
+  build_started => \&Hydra::Event::BuildStarted::parse,
+  step_finished => \&Hydra::Event::StepFinished::parse,
+  build_finished => \&Hydra::Event::BuildFinished::parse,
+);
+
+
+sub parse_payload :prototype($$) {
+    my ($channel_name, $payload) = @_;
+    my @payload = split /\t/, $payload;
+
+    my $parser = %channels_to_events{$channel_name};
+    unless (defined $parser) {
+      die "Invalid channel name: '$channel_name'";
+    }
+
+    return $parser->(@payload);
+}
diff --git a/src/lib/Hydra/Event/BuildFinished.pm b/src/lib/Hydra/Event/BuildFinished.pm
new file mode 100644
index 00000000..50ac2054
--- /dev/null
+++ b/src/lib/Hydra/Event/BuildFinished.pm
@@ -0,0 +1,25 @@
+package Hydra::Event::BuildFinished;
+
+use strict;
+use warnings;
+
+sub parse :prototype(@) {
+    if (@_ == 0) {
+        die "build_finished: payload takes at least one argument, but ", scalar(@_), " were given";
+    }
+
+    my @failures = grep(!/^\d+$/, @_);
+    if (@failures > 0) {
+        die "build_finished: payload arguments should be integers, but we received the following non-integers:", @failures;
+    }
+
+    my ($build_id, @dependents) = map int, @_;
+    return Hydra::Event::BuildFinished->new($build_id, \@dependents);
+}
+
+sub new {
+    my ($self, $build_id, $dependencies) = @_;
+    return bless { "build_id" => $build_id, "dependencies" => $dependencies }, $self;
+}
+
+1;
diff --git a/src/lib/Hydra/Event/BuildStarted.pm b/src/lib/Hydra/Event/BuildStarted.pm
new file mode 100644
index 00000000..27284387
--- /dev/null
+++ b/src/lib/Hydra/Event/BuildStarted.pm
@@ -0,0 +1,25 @@
+package Hydra::Event::BuildStarted;
+
+use strict;
+use warnings;
+
+sub parse :prototype(@) {
+    unless (@_ == 1) {
+        die "build_started: payload takes only one argument, but ", scalar(@_), " were given";
+    }
+
+    my ($build_id) = @_;
+
+    unless ($build_id =~ /^\d+$/) {
+        die "build_started: payload argument should be an integer, but '", $build_id, "' was given"
+    }
+
+    return Hydra::Event::BuildStarted->new(int($build_id));
+}
+
+sub new {
+    my ($self, $id) = @_;
+    return bless { "build_id" => $id }, $self;
+}
+
+1;
diff --git a/src/lib/Hydra/Event/StepFinished.pm b/src/lib/Hydra/Event/StepFinished.pm
new file mode 100644
index 00000000..0e6d26c1
--- /dev/null
+++ b/src/lib/Hydra/Event/StepFinished.pm
@@ -0,0 +1,29 @@
+package Hydra::Event::StepFinished;
+
+use strict;
+use warnings;
+
+
+sub parse :prototype(@) {
+    unless (@_ == 3) {
+        die "step_finished: payload takes exactly three arguments, but ", scalar(@_), " were given";
+    }
+
+    my ($build_id, $step_number, $log_path) = @_;
+
+    unless ($build_id =~ /^\d+$/) {
+        die "step_finished: payload argument build_id should be an integer, but '", $build_id, "' was given"
+    }
+    unless ($step_number =~ /^\d+$/) {
+        die "step_finished: payload argument step_number should be an integer, but '", $step_number, "' was given"
+    }
+
+    return Hydra::Event::StepFinished->new(int($build_id), int($step_number), $log_path);
+}
+
+sub new :prototype($$$) {
+    my ($self, $build_id, $step_number, $log_path) = @_;
+    return bless { "build_id" => $build_id, "step_number" => $step_number, "log_path" => $log_path }, $self;
+}
+
+1;
diff --git a/t/Event.t b/t/Event.t
new file mode 100644
index 00000000..95feb29a
--- /dev/null
+++ b/t/Event.t
@@ -0,0 +1,113 @@
+use strict;
+use Hydra::Event;
+use Hydra::Event::BuildFinished;
+use Hydra::Event::BuildStarted;
+use Hydra::Event::StepFinished;
+
+use Test2::V0;
+use Test2::Tools::Exception;
+
+subtest "Payload type: build_started" => sub {
+    like(
+        dies { Hydra::Event::parse_payload("build_started", "") },
+        qr/one argument/,
+        "empty payload"
+    );
+    like(
+        dies { Hydra::Event::parse_payload("build_started", "abc123\tabc123") },
+        qr/only one argument/,
+        "two arguments"
+    );
+
+    like(
+        dies { Hydra::Event::parse_payload("build_started", "abc123") },
+        qr/should be an integer/,
+        "not an integer"
+    );
+    is(
+        Hydra::Event::parse_payload("build_started", "19"),
+        Hydra::Event::BuildStarted->new(19),
+        "Valid parse"
+    );
+};
+
+subtest "Payload type: step_finished" => sub {
+    like(
+        dies { Hydra::Event::parse_payload("step_finished", "") },
+        qr/three arguments/,
+        "empty payload"
+    );
+    like(
+        dies { Hydra::Event::parse_payload("step_finished", "abc123") },
+        qr/three arguments/,
+        "one argument"
+    );
+    like(
+        dies { Hydra::Event::parse_payload("step_finished", "abc123\tabc123") },
+        qr/three arguments/,
+        "two arguments"
+    );
+    like(
+        dies { Hydra::Event::parse_payload("step_finished", "abc123\tabc123\tabc123\tabc123") },
+        qr/three arguments/,
+        "four arguments"
+    );
+    like(
+        dies { Hydra::Event::parse_payload("step_finished", "abc123\t123\t/path/to/log") },
+        qr/should be an integer/,
+        "not an integer: first position"
+    );
+    like(
+        dies { Hydra::Event::parse_payload("step_finished", "123\tabc123\t/path/to/log") },
+        qr/should be an integer/,
+        "not an integer: second argument"
+    );
+    is(
+        Hydra::Event::parse_payload("step_finished", "123\t456\t/path/to/logfile"),
+        Hydra::Event::StepFinished->new(123, 456, "/path/to/logfile")
+    );
+};
+
+subtest "Payload type: build_finished" => sub {
+    like(
+        dies { Hydra::Event::parse_payload("build_finished", "") },
+        qr/at least one argument/,
+        "empty payload"
+    );
+    like(
+        dies { Hydra::Event::parse_payload("build_finished", "abc123") },
+        qr/should be integers/,
+        "build ID should be an integer"
+    );
+    like(
+        dies { Hydra::Event::parse_payload("build_finished", "123\tabc123") },
+        qr/should be integers/,
+        "dependent ID should be an integer"
+    );
+    is(
+        Hydra::Event::parse_payload("build_finished", "123"),
+        Hydra::Event::BuildFinished->new(123, []),
+        "no dependent builds"
+    );
+    is(
+        Hydra::Event::parse_payload("build_finished", "123\t456"),
+        Hydra::Event::BuildFinished->new(123, [456]),
+        "one dependent build"
+    );
+    is(
+        Hydra::Event::parse_payload("build_finished", "123\t456\t789\t012\t345"),
+        Hydra::Event::BuildFinished->new(123, [456, 789, 12, 345]),
+        "four dependent builds"
+    );
+};
+
+
+subtest "Payload type: bogus" => sub {
+    like(
+        dies { Hydra::Event::parse_payload("bogus", "") },
+        qr/Invalid channel name/,
+        "bogus channel"
+    );
+};
+
+done_testing;

From 4e86e55008075d2fc57453521c59f86d713b28b2 Mon Sep 17 00:00:00 2001
From: Graham Christensen <graham@grahamc.com>
Date: Mon, 9 Aug 2021 10:05:33 -0400
Subject: [PATCH 2/7] Event.pm: add a new_event helper to parse and construct
 an Event

An Event will be part of many Tasks
---
 src/lib/Hydra/Event.pm | 16 +++++++++++-----
 t/Event.t              |  7 +++++++
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/src/lib/Hydra/Event.pm b/src/lib/Hydra/Event.pm
index ac1222e0..8608c87b 100644
--- a/src/lib/Hydra/Event.pm
+++ b/src/lib/Hydra/Event.pm
@@ -1,14 +1,9 @@
 package Hydra::Event;
 
-
 use strict;
 use Hydra::Event::BuildFinished;
 use Hydra::Event::BuildStarted;
 use Hydra::Event::StepFinished;
-our @ISA = qw(Exporter);
-our @EXPORT = qw(
-    parse_payload
-);
 
 my %channels_to_events = (
   build_started => \&Hydra::Event::BuildStarted::parse,
@@ -28,3 +23,14 @@ sub parse_payload :prototype($$) {
 
     return $parser->(@payload);
 }
+
+
+sub new_event {
+    my ($self, $channel_name, $payload) = @_;
+
+    return bless {
+        "channel_name" => $channel_name,
+        "payload" => $payload,
+        "event" => parse_payload($channel_name, $payload),
+    }, $self;
+}
diff --git a/t/Event.t b/t/Event.t
index 95feb29a..849070f8 100644
--- a/t/Event.t
+++ b/t/Event.t
@@ -7,6 +7,13 @@ use Hydra::Event::StepFinished;
 use Test2::V0;
 use Test2::Tools::Exception;
 
+subtest "Event: new event" => sub {
+    my $event = Hydra::Event->new_event("build_started", "19");
+    is($event->{'payload'}, "19");
+    is($event->{'channel_name'}, "build_started");
+    is($event->{'event'}->{'build_id'}, 19);
+};
+
 subtest "Payload type: build_started" => sub {
     like(
         dies { Hydra::Event::parse_payload("build_started", "") },

From d02c6794f449f3b1feed03006f9a983f609e04ef Mon Sep 17 00:00:00 2001
From: Graham Christensen <graham@grahamc.com>
Date: Wed, 11 Aug 2021 12:42:17 -0400
Subject: [PATCH 3/7] Event: teach how to execute a plugin

---
 src/lib/Hydra/Event.pm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/lib/Hydra/Event.pm b/src/lib/Hydra/Event.pm
index 8608c87b..cd8bdde2 100644
--- a/src/lib/Hydra/Event.pm
+++ b/src/lib/Hydra/Event.pm
@@ -34,3 +34,8 @@ sub new_event {
         "event" => parse_payload($channel_name, $payload),
     }, $self;
 }
+
+sub execute {
+    my ($self, $db, $plugin) = @_;
+    return $self->{"event"}->execute($db, $plugin);
+}

From 10e85e3422e1ad9f3918654af311f44eae32c03c Mon Sep 17 00:00:00 2001
From: Graham Christensen <graham@grahamc.com>
Date: Thu, 12 Aug 2021 12:53:58 -0400
Subject: [PATCH 4/7] hydra-notify: Create a helper for running each plugin on
 an event

---
 src/script/hydra-notify | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/src/script/hydra-notify b/src/script/hydra-notify
index 1c696cac..904ec334 100755
--- a/src/script/hydra-notify
+++ b/src/script/hydra-notify
@@ -31,6 +31,19 @@ $listener->subscribe("build_started");
 $listener->subscribe("build_finished");
 $listener->subscribe("step_finished");
 
+sub runPluginsForEvent {
+    my ($event) = @_;
+
+    foreach my $plugin (@plugins) {
+        eval {
+            $event->execute($db, $plugin);
+            1;
+        } or do {
+            print STDERR "error running $event->{'channel_name'} hooks: $@\n";
+        }
+    }
+}
+
 sub buildStarted {
     my ($buildId) = @_;
 

From 4fdb20d3bdb962a8ad6307b9219295b973f7e17f Mon Sep 17 00:00:00 2001
From: Graham Christensen <graham@grahamc.com>
Date: Thu, 12 Aug 2021 09:56:42 -0400
Subject: [PATCH 5/7] hydra-notify: move BuildStarted processing to an Event

---
 src/lib/Hydra/Event/BuildStarted.pm | 24 ++++++++-
 src/script/hydra-notify             | 19 +------
 t/Event.t                           | 24 ---------
 t/Event/BuildStarted.t              | 77 +++++++++++++++++++++++++++++
 4 files changed, 102 insertions(+), 42 deletions(-)
 create mode 100644 t/Event/BuildStarted.t

diff --git a/src/lib/Hydra/Event/BuildStarted.pm b/src/lib/Hydra/Event/BuildStarted.pm
index 27284387..67fe38ef 100644
--- a/src/lib/Hydra/Event/BuildStarted.pm
+++ b/src/lib/Hydra/Event/BuildStarted.pm
@@ -19,7 +19,29 @@ sub parse :prototype(@) {
 
 sub new {
     my ($self, $id) = @_;
-    return bless { "build_id" => $id }, $self;
+    return bless {
+        "build_id" => $id,
+        "build" => undef
+    }, $self;
+}
+
+sub load {
+    my ($self, $db) = @_;
+
+    if (!defined($self->{"build"})) {
+        $self->{"build"} = $db->resultset('Builds')->find($self->{"build_id"})
+            or die "build $self->{'build_id'} does not exist\n";
+    }
+}
+
+sub execute {
+    my ($self, $db, $plugin) = @_;
+
+    $self->load($db);
+
+    $plugin->buildStarted($self->{"build"});
+
+    return 1;
 }
 
 1;
diff --git a/src/script/hydra-notify b/src/script/hydra-notify
index 904ec334..99618101 100755
--- a/src/script/hydra-notify
+++ b/src/script/hydra-notify
@@ -44,22 +44,6 @@ sub runPluginsForEvent {
     }
 }
 
-sub buildStarted {
-    my ($buildId) = @_;
-
-    my $build = $db->resultset('Builds')->find($buildId)
-        or die "build $buildId does not exist\n";
-
-    foreach my $plugin (@plugins) {
-        eval {
-            $plugin->buildStarted($build);
-            1;
-        } or do {
-            print STDERR "error with $plugin->buildStarted: $@\n";
-        }
-    }
-}
-
 sub buildFinished {
     my ($buildId, @deps) = @_;
 
@@ -136,7 +120,8 @@ while (!$queued_only) {
 
         eval {
             if ($channelName eq "build_started") {
-                buildStarted(int($payload[0]));
+                my $event = Hydra::Event::new_event($channelName, $message->{"payload"});
+                runPluginsForEvent($event);
             } elsif ($channelName eq "build_finished") {
                 my $buildId = int($payload[0]);
                 buildFinished($buildId, @payload[1..$#payload]);
diff --git a/t/Event.t b/t/Event.t
index 849070f8..7a8023cd 100644
--- a/t/Event.t
+++ b/t/Event.t
@@ -1,7 +1,6 @@
 use strict;
 use Hydra::Event;
 use Hydra::Event::BuildFinished;
-use Hydra::Event::BuildStarted;
 use Hydra::Event::StepFinished;
 
 use Test2::V0;
@@ -14,29 +13,6 @@ subtest "Event: new event" => sub {
     is($event->{'event'}->{'build_id'}, 19);
 };
 
-subtest "Payload type: build_started" => sub {
-    like(
-        dies { Hydra::Event::parse_payload("build_started", "") },
-        qr/one argument/,
-        "empty payload"
-    );
-    like(
-        dies { Hydra::Event::parse_payload("build_started", "abc123\tabc123") },
-        qr/only one argument/,
-        "two arguments"
-    );
-
-    like(
-        dies { Hydra::Event::parse_payload("build_started", "abc123") },
-        qr/should be an integer/,
-        "not an integer"
-    );
-    is(
-        Hydra::Event::parse_payload("build_started", "19"),
-        Hydra::Event::BuildStarted->new(19),
-        "Valid parse"
-    );
-};
 
 subtest "Payload type: step_finished" => sub {
     like(
diff --git a/t/Event/BuildStarted.t b/t/Event/BuildStarted.t
new file mode 100644
index 00000000..04a7df54
--- /dev/null
+++ b/t/Event/BuildStarted.t
@@ -0,0 +1,77 @@
+use strict;
+use Setup;
+
+my %ctx = test_init();
+
+require Hydra::Schema;
+require Hydra::Model::DB;
+use Hydra::Event;
+use Hydra::Event::BuildStarted;
+
+use Test2::V0;
+use Test2::Tools::Exception;
+use Test2::Tools::Mock qw(mock_obj);
+
+my $db = Hydra::Model::DB->new;
+hydra_setup($db);
+
+my $project = $db->resultset('Projects')->create({name => "tests", displayname => "", owner => "root"});
+my $jobset = createBaseJobset("basic", "basic.nix", $ctx{jobsdir});
+ok(evalSucceeds($jobset),               "Evaluating jobs/basic.nix should exit with return code 0");
+is(nrQueuedBuildsForJobset($jobset), 3, "Evaluating jobs/basic.nix should result in 3 builds");
+
+subtest "Parsing build_started" => sub {
+    like(
+        dies { Hydra::Event::parse_payload("build_started", "") },
+        qr/one argument/,
+        "empty payload"
+    );
+    like(
+        dies { Hydra::Event::parse_payload("build_started", "abc123\tabc123") },
+        qr/only one argument/,
+        "two arguments"
+    );
+
+    like(
+        dies { Hydra::Event::parse_payload("build_started", "abc123") },
+        qr/should be an integer/,
+        "not an integer"
+    );
+    is(
+        Hydra::Event::parse_payload("build_started", "19"),
+        Hydra::Event::BuildStarted->new(19),
+        "Valid parse"
+    );
+};
+
+subtest "load" => sub {
+    my $build = $db->resultset('Builds')->search(
+      { },
+      { limit => 1 }
+    )->next;
+
+    my $event = Hydra::Event::BuildStarted->new($build->id);
+
+    $event->load($db);
+
+    is($event->{"build"}->id, $build->id, "The build record matches.");
+
+    # Create a fake "plugin" with a buildStarted sub, the sub sets this
+    # global passedBuild variable.
+    my $passedBuild;
+    my $plugin = {};
+    my $mock = mock_obj $plugin => (
+        add => [
+            "buildStarted" => sub {
+                my ($self, $build) = @_;
+                $passedBuild = $build;
+            }
+        ]
+    );
+
+    $event->execute($db, $plugin);
+
+    is($passedBuild->id, $build->id, "The plugin's buildStarted hook is called with the proper build");
+};
+
+done_testing;

From 4a1389e36ef5af2513d58eeaa3a5940c453b108f Mon Sep 17 00:00:00 2001
From: Graham Christensen <graham@grahamc.com>
Date: Thu, 12 Aug 2021 10:28:35 -0400
Subject: [PATCH 6/7] hydra-notify: move StepFinished processing to an Event

---
 src/lib/Hydra/Event/StepFinished.pm |  32 ++++++++-
 src/script/hydra-notify             |  25 +------
 t/Event.t                           |  38 -----------
 t/Event/StepFinished.t              | 101 ++++++++++++++++++++++++++++
 4 files changed, 133 insertions(+), 63 deletions(-)
 create mode 100644 t/Event/StepFinished.t

diff --git a/src/lib/Hydra/Event/StepFinished.pm b/src/lib/Hydra/Event/StepFinished.pm
index 0e6d26c1..d14423ad 100644
--- a/src/lib/Hydra/Event/StepFinished.pm
+++ b/src/lib/Hydra/Event/StepFinished.pm
@@ -23,7 +23,37 @@ sub parse :prototype(@) {
 
 sub new :prototype($$$) {
     my ($self, $build_id, $step_number, $log_path) = @_;
-    return bless { "build_id" => $build_id, "step_number" => $step_number, "log_path" => $log_path }, $self;
+
+    $log_path = undef if $log_path eq "-";
+
+    return bless {
+        "build_id" => $build_id,
+        "step_number" => $step_number,
+        "log_path" => $log_path,
+        "step" => undef,
+    }, $self;
+}
+
+sub load {
+    my ($self, $db) = @_;
+
+    if (!defined($self->{"step"})) {
+        my $build = $db->resultset('Builds')->find($self->{"build_id"})
+            or die "build $self->{'build_id'} does not exist\n";
+
+        $self->{"step"} = $build->buildsteps->find({stepnr => $self->{"step_number"}})
+            or die "step $self->{'step_number'} does not exist\n";
+    }
+}
+
+sub execute {
+    my ($self, $db, $plugin) = @_;
+
+    $self->load($db);
+
+    $plugin->stepFinished($self->{"step"}, $self->{"log_path"});
+
+    return 1;
 }
 
 1;
diff --git a/src/script/hydra-notify b/src/script/hydra-notify
index 99618101..7ecb7ddf 100755
--- a/src/script/hydra-notify
+++ b/src/script/hydra-notify
@@ -75,27 +75,6 @@ sub buildFinished {
     }
 }
 
-sub stepFinished {
-    my ($buildId, $stepNr, $logPath) = @_;
-
-    my $build = $db->resultset('Builds')->find($buildId)
-        or die "build $buildId does not exist\n";
-
-    my $step = $build->buildsteps->find({stepnr => $stepNr})
-        or die "step $stepNr does not exist\n";
-
-    $logPath = undef if $logPath eq "-";
-
-    foreach my $plugin (@plugins) {
-        eval {
-            $plugin->stepFinished($step, $logPath);
-            1;
-        } or do {
-            print STDERR "error with $plugin->stepFinished: $@\n";
-        }
-    }
-}
-
 # Process builds that finished while hydra-notify wasn't running.
 for my $build ($db->resultset('Builds')->search(
                    { notificationpendingsince => { '!=', undef } }))
@@ -119,14 +98,12 @@ while (!$queued_only) {
         my @payload = split /\t/, $payload;
 
         eval {
-            if ($channelName eq "build_started") {
+            if ($channelName eq "build_started" || $channelName eq "step_finished" ) {
                 my $event = Hydra::Event::new_event($channelName, $message->{"payload"});
                 runPluginsForEvent($event);
             } elsif ($channelName eq "build_finished") {
                 my $buildId = int($payload[0]);
                 buildFinished($buildId, @payload[1..$#payload]);
-            } elsif ($channelName eq "step_finished") {
-                stepFinished(int($payload[0]), int($payload[1]));
             }
             1;
         } or do {
diff --git a/t/Event.t b/t/Event.t
index 7a8023cd..d70c661e 100644
--- a/t/Event.t
+++ b/t/Event.t
@@ -1,7 +1,6 @@
 use strict;
 use Hydra::Event;
 use Hydra::Event::BuildFinished;
-use Hydra::Event::StepFinished;
 
 use Test2::V0;
 use Test2::Tools::Exception;
@@ -14,43 +13,6 @@ subtest "Event: new event" => sub {
 };
 
 
-subtest "Payload type: step_finished" => sub {
-    like(
-        dies { Hydra::Event::parse_payload("step_finished", "") },
-        qr/three arguments/,
-        "empty payload"
-    );
-    like(
-        dies { Hydra::Event::parse_payload("step_finished", "abc123") },
-        qr/three arguments/,
-        "one argument"
-    );
-    like(
-        dies { Hydra::Event::parse_payload("step_finished", "abc123\tabc123") },
-        qr/three arguments/,
-        "two arguments"
-    );
-    like(
-        dies { Hydra::Event::parse_payload("step_finished", "abc123\tabc123\tabc123\tabc123") },
-        qr/three arguments/,
-        "four arguments"
-    );
-    like(
-        dies { Hydra::Event::parse_payload("step_finished", "abc123\t123\t/path/to/log") },
-        qr/should be an integer/,
-        "not an integer: first position"
-    );
-    like(
-        dies { Hydra::Event::parse_payload("step_finished", "123\tabc123\t/path/to/log") },
-        qr/should be an integer/,
-        "not an integer: second argument"
-    );
-    is(
-        Hydra::Event::parse_payload("step_finished", "123\t456\t/path/to/logfile"),
-        Hydra::Event::StepFinished->new(123, 456, "/path/to/logfile")
-    );
-};
-
 subtest "Payload type: build_finished" => sub {
     like(
         dies { Hydra::Event::parse_payload("build_finished", "") },
diff --git a/t/Event/StepFinished.t b/t/Event/StepFinished.t
new file mode 100644
index 00000000..a4bc3fb9
--- /dev/null
+++ b/t/Event/StepFinished.t
@@ -0,0 +1,101 @@
+use strict;
+use Setup;
+
+my %ctx = test_init();
+
+require Hydra::Schema;
+require Hydra::Model::DB;
+use Hydra::Event;
+use Hydra::Event::BuildStarted;
+
+use Test2::V0;
+use Test2::Tools::Exception;
+use Test2::Tools::Mock qw(mock_obj);
+
+my $db = Hydra::Model::DB->new;
+hydra_setup($db);
+
+my $project = $db->resultset('Projects')->create({name => "tests", displayname => "", owner => "root"});
+my $jobset = createBaseJobset("basic", "basic.nix", $ctx{jobsdir});
+ok(evalSucceeds($jobset),               "Evaluating jobs/basic.nix should exit with return code 0");
+is(nrQueuedBuildsForJobset($jobset), 3, "Evaluating jobs/basic.nix should result in 3 builds");
+
+for my $build (queuedBuildsForJobset($jobset)) {
+    ok(runBuild($build), "Build '".$build->job."' from jobs/basic.nix should exit with return code 0");
+}
+
+
+
+subtest "Parsing step_finished" => sub {
+    like(
+        dies { Hydra::Event::parse_payload("step_finished", "") },
+        qr/three arguments/,
+        "empty payload"
+    );
+    like(
+        dies { Hydra::Event::parse_payload("step_finished", "abc123") },
+        qr/three arguments/,
+        "one argument"
+    );
+    like(
+        dies { Hydra::Event::parse_payload("step_finished", "abc123\tabc123") },
+        qr/three arguments/,
+        "two arguments"
+    );
+    like(
+        dies { Hydra::Event::parse_payload("step_finished", "abc123\tabc123\tabc123\tabc123") },
+        qr/three arguments/,
+        "four arguments"
+    );
+    like(
+        dies { Hydra::Event::parse_payload("step_finished", "abc123\t123\t/path/to/log") },
+        qr/should be an integer/,
+        "not an integer: first position"
+    );
+    like(
+        dies { Hydra::Event::parse_payload("step_finished", "123\tabc123\t/path/to/log") },
+        qr/should be an integer/,
+        "not an integer: second argument"
+    );
+    is(
+        Hydra::Event::parse_payload("step_finished", "123\t456\t/path/to/logfile"),
+        Hydra::Event::StepFinished->new(123, 456, "/path/to/logfile")
+    );
+};
+
+subtest "load" => sub {
+
+    my $step = $db->resultset('BuildSteps')->search(
+      { },
+      { limit => 1 }
+    )->next;
+    my $build = $step->build;
+
+    my $event = Hydra::Event::StepFinished->new($build->id, $step->stepnr, "/foo/bar/baz");
+
+    $event->load($db);
+    is($event->{"step"}->get_column("build"), $build->id, "The build record matches.");
+
+    # Create a fake "plugin" with a stepFinished sub, the sub sets this
+    # "global" passedStep, passedLogPath variables.
+    my $passedStep;
+    my $passedLogPath;
+    my $plugin = {};
+    my $mock = mock_obj $plugin => (
+        add => [
+            "stepFinished" => sub {
+                my ($self, $step, $log_path) = @_;
+                $passedStep = $step;
+                $passedLogPath = $log_path;
+            }
+        ]
+    );
+
+    $event->execute($db, $plugin);
+
+    is($passedStep->get_column("build"), $build->id, "The plugin's stepFinished hook is called with a step from the expected build");
+    is($passedStep->stepnr, $step->stepnr, "The plugin's stepFinished hook is called with the proper step of the build");
+    is($passedLogPath, "/foo/bar/baz", "The plugin's stepFinished hook is called with the proper log path");
+};
+
+done_testing;

From fa6d7abc13d00455109c1d722dc72ff0bca772c3 Mon Sep 17 00:00:00 2001
From: Graham Christensen <graham@grahamc.com>
Date: Thu, 12 Aug 2021 12:03:25 -0400
Subject: [PATCH 7/7] hydra-notify: move BuildFinished processing to an Event

---
 src/lib/Hydra/Event/BuildFinished.pm | 44 ++++++++++++-
 src/script/hydra-notify              | 57 ++++-------------
 t/Event.t                            | 36 -----------
 t/Event/BuildFinished.t              | 92 ++++++++++++++++++++++++++++
 4 files changed, 145 insertions(+), 84 deletions(-)
 create mode 100644 t/Event/BuildFinished.t

diff --git a/src/lib/Hydra/Event/BuildFinished.pm b/src/lib/Hydra/Event/BuildFinished.pm
index 50ac2054..dc94b5ee 100644
--- a/src/lib/Hydra/Event/BuildFinished.pm
+++ b/src/lib/Hydra/Event/BuildFinished.pm
@@ -18,8 +18,48 @@ sub parse :prototype(@) {
 }
 
 sub new {
-    my ($self, $build_id, $dependencies) = @_;
-    return bless { "build_id" => $build_id, "dependencies" => $dependencies }, $self;
+    my ($self, $build_id, $dependent_ids) = @_;
+    return bless {
+        "build_id" => $build_id,
+        "dependent_ids" => $dependent_ids,
+        "build" => undef,
+        "dependents" => [],
+    }, $self;
+}
+
+sub load {
+    my ($self, $db) = @_;
+
+    if (!defined($self->{"build"})) {
+        $self->{"build"} = $db->resultset('Builds')->find($self->{"build_id"})
+            or die "build $self->{'build_id'} does not exist\n";
+
+        foreach my $id (@{$self->{"dependent_ids"}}) {
+            my $dep = $db->resultset('Builds')->find($id)
+                or die "dependent build $id does not exist\n";
+            push @{$self->{"dependents"}}, $dep;
+        }
+    }
+}
+
+sub execute {
+    my ($self, $db, $plugin) = @_;
+
+    $self->load($db);
+
+    $plugin->buildFinished($self->{"build"}, $self->{"dependents"});
+
+    # Mark the build and all dependents as having their notifications "finished".
+    #
+    # Otherwise, the dependent builds will remain with notificationpendingsince set
+    # until hydra-notify is started, as buildFinished is never emitted for them.
+    foreach my $b ($self->{"build"}, @{$self->{"dependents"}}) {
+        if ($b->finished && defined($b->notificationpendingsince)) {
+            $b->update({ notificationpendingsince => undef })
+        }
+    }
+
+    return 1;
 }
 
 1;
diff --git a/src/script/hydra-notify b/src/script/hydra-notify
index 7ecb7ddf..2c323b7f 100755
--- a/src/script/hydra-notify
+++ b/src/script/hydra-notify
@@ -2,10 +2,11 @@
 
 use strict;
 use utf8;
+use Hydra::Event::BuildFinished;
+use Hydra::Helper::AddBuilds;
+use Hydra::Helper::Nix;
 use Hydra::Plugin;
 use Hydra::PostgresListener;
-use Hydra::Helper::Nix;
-use Hydra::Helper::AddBuilds;
 use Getopt::Long;
 
 STDERR->autoflush(1);
@@ -44,44 +45,15 @@ sub runPluginsForEvent {
     }
 }
 
-sub buildFinished {
-    my ($buildId, @deps) = @_;
-
-    my $build = $db->resultset('Builds')->find($buildId)
-    or die "build $buildId does not exist\n";
-
-    my @dependents;
-    foreach my $id (@deps) {
-        my $dep = $db->resultset('Builds')->find($id)
-            or die "build $id does not exist\n";
-        push @dependents, $dep;
-    }
-
-    foreach my $plugin (@plugins) {
-        eval {
-            $plugin->buildFinished($build, [@dependents]);
-            1;
-        } or do {
-            print STDERR "error with $plugin->buildFinished: $@\n";
-        }
-    }
-
-    # We have to iterate through all dependents as well, and if they are finished
-    # to mark their notificationpendingsince.
-    # Otherwise, the dependent builds will remain with notificationpendingsince set
-    # until hydra-notify is started, as buildFinished is never emitted for them.
-    foreach my $b ($build, @dependents) {
-        $b->update({ notificationpendingsince => undef }) if $b->finished;
-    }
-}
-
 # Process builds that finished while hydra-notify wasn't running.
 for my $build ($db->resultset('Builds')->search(
                    { notificationpendingsince => { '!=', undef } }))
 {
-    my $buildId = $build->id;
-    print STDERR "sending notifications for build ${\$buildId}...\n";
-    buildFinished($build->id);
+    print STDERR "sending notifications for build $build->id...\n";
+
+
+    my $event = Hydra::Event::BuildFinished->new($build->id);
+    runPluginsForEvent($event);
 }
 
 
@@ -93,18 +65,11 @@ while (!$queued_only) {
         my $channelName = $message->{"channel"};
         my $pid = $message->{"pid"};
         my $payload = $message->{"payload"};
-        #print STDERR "got '$channelName' from $pid: $payload\n";
-
-        my @payload = split /\t/, $payload;
 
         eval {
-            if ($channelName eq "build_started" || $channelName eq "step_finished" ) {
-                my $event = Hydra::Event::new_event($channelName, $message->{"payload"});
-                runPluginsForEvent($event);
-            } elsif ($channelName eq "build_finished") {
-                my $buildId = int($payload[0]);
-                buildFinished($buildId, @payload[1..$#payload]);
-            }
+            my $event = Hydra::Event::new_event($channelName, $message->{"payload"});
+            runPluginsForEvent($event);
+
             1;
         } or do {
             print STDERR "error processing message '$payload' on channel '$channelName': $@\n";
diff --git a/t/Event.t b/t/Event.t
index d70c661e..a15b95ea 100644
--- a/t/Event.t
+++ b/t/Event.t
@@ -1,6 +1,5 @@
 use strict;
 use Hydra::Event;
-use Hydra::Event::BuildFinished;
 
 use Test2::V0;
 use Test2::Tools::Exception;
@@ -12,41 +11,6 @@ subtest "Event: new event" => sub {
     is($event->{'event'}->{'build_id'}, 19);
 };
 
-
-subtest "Payload type: build_finished" => sub {
-    like(
-        dies { Hydra::Event::parse_payload("build_finished", "") },
-        qr/at least one argument/,
-        "empty payload"
-    );
-    like(
-        dies { Hydra::Event::parse_payload("build_finished", "abc123") },
-        qr/should be integers/,
-        "build ID should be an integer"
-    );
-    like(
-        dies { Hydra::Event::parse_payload("build_finished", "123\tabc123") },
-        qr/should be integers/,
-        "dependent ID should be an integer"
-    );
-    is(
-        Hydra::Event::parse_payload("build_finished", "123"),
-        Hydra::Event::BuildFinished->new(123, []),
-        "no dependent builds"
-    );
-    is(
-        Hydra::Event::parse_payload("build_finished", "123\t456"),
-        Hydra::Event::BuildFinished->new(123, [456]),
-        "one dependent build"
-    );
-    is(
-        Hydra::Event::parse_payload("build_finished", "123\t456\t789\t012\t345"),
-        Hydra::Event::BuildFinished->new(123, [456, 789, 12, 345]),
-        "four dependent builds"
-    );
-};
-
-
 subtest "Payload type: bogus" => sub {
     like(
         dies { Hydra::Event::parse_payload("bogus", "") },
diff --git a/t/Event/BuildFinished.t b/t/Event/BuildFinished.t
new file mode 100644
index 00000000..8650f95c
--- /dev/null
+++ b/t/Event/BuildFinished.t
@@ -0,0 +1,92 @@
+use strict;
+use Setup;
+
+my %ctx = test_init();
+
+require Hydra::Schema;
+require Hydra::Model::DB;
+use Hydra::Event;
+use Hydra::Event::BuildFinished;
+
+use Test2::V0;
+use Test2::Tools::Exception;
+use Test2::Tools::Mock qw(mock_obj);
+
+my $db = Hydra::Model::DB->new;
+hydra_setup($db);
+
+subtest "Parsing" => sub {
+    like(
+        dies { Hydra::Event::parse_payload("build_finished", "") },
+        qr/at least one argument/,
+        "empty payload"
+    );
+    like(
+        dies { Hydra::Event::parse_payload("build_finished", "abc123") },
+        qr/should be integers/,
+        "build ID should be an integer"
+    );
+    like(
+        dies { Hydra::Event::parse_payload("build_finished", "123\tabc123") },
+        qr/should be integers/,
+        "dependent ID should be an integer"
+    );
+    is(
+        Hydra::Event::parse_payload("build_finished", "123"),
+        Hydra::Event::BuildFinished->new(123, []),
+        "no dependent builds"
+    );
+    is(
+        Hydra::Event::parse_payload("build_finished", "123\t456"),
+        Hydra::Event::BuildFinished->new(123, [456]),
+        "one dependent build"
+    );
+    is(
+        Hydra::Event::parse_payload("build_finished", "123\t456\t789\t012\t345"),
+        Hydra::Event::BuildFinished->new(123, [456, 789, 12, 345]),
+        "four dependent builds"
+    );
+};
+
+my $project = $db->resultset('Projects')->create({name => "tests", displayname => "", owner => "root"});
+my $jobset = createBaseJobset("basic", "basic.nix", $ctx{jobsdir});
+ok(evalSucceeds($jobset),               "Evaluating jobs/basic.nix should exit with return code 0");
+is(nrQueuedBuildsForJobset($jobset), 3, "Evaluating jobs/basic.nix should result in 3 builds");
+
+subtest "load" => sub {
+    my ($build, $dependent_a, $dependent_b) = $db->resultset('Builds')->search(
+      { },
+      { limit => 3 }
+    )->all;
+
+    my $event = Hydra::Event::BuildFinished->new($build->id, [$dependent_a->id, $dependent_b->id]);
+
+    $event->load($db);
+
+    is($event->{"build"}->id, $build->id, "The build record matches.");
+    is($event->{"dependents"}[0]->id, $dependent_a->id, "The dependent_a record matches.");
+    is($event->{"dependents"}[1]->id, $dependent_b->id, "The dependent_b record matches.");
+
+    # Create a fake "plugin" with a buildFinished sub, the sub sets this
+    # global passedBuild and passedDependents variables for verifying.
+    my $passedBuild;
+    my $passedDependents;
+    my $plugin = {};
+    my $mock = mock_obj $plugin => (
+        add => [
+            "buildFinished" => sub {
+                my ($self, $build, $dependents) = @_;
+                $passedBuild = $build;
+                $passedDependents = $dependents;
+            }
+        ]
+    );
+
+    $event->execute($db, $plugin);
+
+    is($passedBuild->id, $build->id, "The plugin's buildFinished hook is called with a matching build");
+    is($passedDependents->[0]->id, $dependent_a->id, "The plugin's buildFinished hook is called with a matching dependent_a");
+    is($passedDependents->[1]->id, $dependent_b->id, "The plugin's buildFinished hook is called with a matching dependent_b");
+};
+
+done_testing;