Francois Gouget : testbot/lib: Turn the Jobs::Schedule() and Jobs::Check() methods into functions.

[Alexandre Julliard]

Module: tools
Branch: master
Commit: 73c62ab7dc4bafb83bfc676b018b5db55e5edd3a
URL:    http://source.winehq.org/git/tools.git/?a=commit;h=73c62ab7dc4bafb83bfc676b0...

Author: Francois Gouget <fgouget(a)codeweavers.com>
Date:   Fri Nov  2 00:11:12 2012 +0100

testbot/lib: Turn the Jobs::Schedule() and Jobs::Check() methods into functions.

Their typical usage pattern shows there is no reason for them to be methods.
This even more true for Check() since it modifies the object it is called on.

---

 testbot/bin/Engine.pl              |   15 ++++-------
 testbot/lib/WineTestBot/Jobs.pm    |   46 ++++++++++++++++-------------------
 testbot/lib/WineTestBot/Patches.pm |    6 ++--
 3 files changed, 30 insertions(+), 37 deletions(-)

diff --git a/testbot/bin/Engine.pl b/testbot/bin/Engine.pl
index 7bd14a1..1cc8a9b 100755
--- a/testbot/bin/Engine.pl
+++ b/testbot/bin/Engine.pl
@@ -81,7 +81,7 @@ sub HandleJobSubmit
     }
   }
 
-  $ErrMessage = $Jobs->Schedule();
+  $ErrMessage = ScheduleJobs();
   if (defined($ErrMessage))
   {
     LogMsg "Scheduling problem in HandleJobSubmit: $ErrMessage\n";
@@ -158,7 +158,7 @@ sub HandleJobCancel
 
 sub HandleTaskComplete
 {
-  my $ErrMessage = CreateJobs()->Schedule();
+  my $ErrMessage = ScheduleJobs();
   if (defined($ErrMessage))
   {
     LogMsg "Scheduling problem in HandleTaskComplete: $ErrMessage\n";
@@ -180,7 +180,7 @@ sub HandleVMStatusChange
   if ($OldStatus eq "reverting" || $OldStatus eq "running" ||
       $NewStatus eq "idle" || $NewStatus eq "dirty")
   {
-    my $ErrMessage = CreateJobs()->Schedule();
+    my $ErrMessage = ScheduleJobs();
     if (defined($ErrMessage))
     {
       LogMsg "Scheduling problem in HandleVMStatusChange: $ErrMessage\n";
@@ -266,7 +266,7 @@ sub HandleFoundWinetestUpdate
     DeleteEvent("GiveUpOnWinetestUpdate");
   }
 
-  my $ErrMessage = CreateJobs()->Schedule();
+  my $ErrMessage = ScheduleJobs();
   if (defined($ErrMessage))
   {
     LogMsg "Scheduling problem in HandleFoundWinetestUpdate: $ErrMessage\n";
@@ -511,11 +511,8 @@ checks whether any pending patchsets are now complete and thus can be scheduled.
 
 sub SafetyNet
 {
-  my $Jobs = CreateJobs();
-  $Jobs->Check();
-  $Jobs = undef;
-  $Jobs = CreateJobs();
-  $Jobs->Schedule();
+  CheckJobs();
+  ScheduleJobs();
 
   if (opendir STAGING, "$DataDir/staging")
   {
diff --git a/testbot/lib/WineTestBot/Jobs.pm b/testbot/lib/WineTestBot/Jobs.pm
index 649f3ca..74547d2 100644
--- a/testbot/lib/WineTestBot/Jobs.pm
+++ b/testbot/lib/WineTestBot/Jobs.pm
@@ -277,7 +277,7 @@ use vars qw(@ISA @EXPORT @PropertyDescriptors);
 
 require Exporter;
 @ISA = qw(WineTestBot::WineTestBotCollection Exporter);
-(a)EXPORT = qw(&CreateJobs);
+(a)EXPORT = qw(&CreateJobs &ScheduleJobs &CheckJobs);
 
 my @PropertyDescriptors;
 
@@ -374,10 +374,9 @@ VM.
 =back
 =cut
 
-sub ScheduleOnHost
+sub ScheduleOnHost($$)
 {
-  my $self = shift;
-  my $Hypervisors = $_[0];
+  my ($SortedJobs, $Hypervisors) = @_;
 
   my $HostVMs = CreateVMs();
   $HostVMs->FilterHypervisor($Hypervisors);
@@ -385,11 +384,8 @@ sub ScheduleOnHost
   my $PoweredOnNonBaseVMs = $HostVMs->CountPoweredOnNonBaseVMs();
   my %DirtyVMsBlockingJobs;
 
-  $self->AddFilter("Status", ["queued", "running"]);
-  my @SortedJobs = sort CompareJobPriority @{$self->GetItems()};
-
   my $DirtyIndex = 0;
-  foreach my $Job (@SortedJobs)
+  foreach my $Job (@$SortedJobs)
   {
     my $Steps = $Job->Steps;
     $Steps->AddFilter("Status", ["queued", "running"]);
@@ -489,7 +485,7 @@ sub ScheduleOnHost
 =pod
 =over 12
 
-=item C<Schedule()>
+=item C<ScheduleJobs()>
 
 Goes through the WineTestBot hosts and schedules the Job tasks on each of
 them using WineTestBot::Jobs::ScheduleOnHost().
@@ -497,9 +493,13 @@ them using WineTestBot::Jobs::ScheduleOnHost().
 =back
 =cut
 
-sub Schedule
+sub ScheduleJobs()
 {
-  my $self = shift;
+  my $Jobs = CreateJobs();
+  $Jobs->AddFilter("Status", ["queued", "running"]);
+  my @SortedJobs = sort CompareJobPriority @{$Jobs->GetItems()};
+  # Note that even if there are no jobs to schedule
+  # we should check if there are VMs to revert
 
   my %Hosts;
   foreach my $VM (@{CreateVMs()->GetItems()})
@@ -507,24 +507,21 @@ sub Schedule
     my $Host = $VM->GetHost();
     $Hosts{$Host}->{$VM->VirtURI} = 1;
   }
-  my $ErrMessage;
+
+  my @ErrMessages;
   foreach my $Host (keys %Hosts)
   {
     my @HostHypervisors = keys %{$Hosts{$Host}};
-    my $HostErrMessage = $self->ScheduleOnHost(\@HostHypervisors);
-    if (! defined($ErrMessage))
-    {
-      $ErrMessage = $HostErrMessage;
-    }
+    my $HostErrMessage = ScheduleOnHost(\@SortedJobs, \@HostHypervisors);
+    push @ErrMessages, $HostErrMessage if (defined $HostErrMessage);
   }
-
-  return $ErrMessage;
+  return @ErrMessages ? join("\n", @ErrMessages) : undef;
 }
 
 =pod
 =over 12
 
-=item C<Check()>
+=item C<CheckJobs()>
 
 Goes through the list of Jobs and updates their status. As a side-effect this
 detects failed builds, dead child processes, etc.
@@ -532,12 +529,11 @@ detects failed builds, dead child processes, etc.
 =back
 =cut
 
-sub Check
+sub CheckJobs()
 {
-  my $self = shift;
-
-  $self->AddFilter("Status", ["queued", "running"]);
-  map { $_->UpdateStatus(); } @{$self->GetItems()};
+  my $Jobs = CreateJobs();
+  $Jobs->AddFilter("Status", ["queued", "running"]);
+  map { $_->UpdateStatus(); } @{$Jobs->GetItems()};
 
   return undef;
 }
diff --git a/testbot/lib/WineTestBot/Patches.pm b/testbot/lib/WineTestBot/Patches.pm
index 2852242..232e767 100644
--- a/testbot/lib/WineTestBot/Patches.pm
+++ b/testbot/lib/WineTestBot/Patches.pm
@@ -110,7 +110,7 @@ Analyzes the current patch to determine which Wine tests are impacted. Then for
 each impacted test it creates a high priority WineTestBot::Job to run that test.
 This also creates the WineTestBot::Step objects for that Job, as well as the
 WineTestBot::Task objects to run the test on each 'base' VM. Finally it calls
-C<WineTestBot::Jobs::Schedule()> to run the new Jobs.
+C<WineTestBot::Jobs::ScheduleJobs()> to run the new Jobs.
 
 Note that the path to the file containing the actual patch is passed as a
 parameter. This is used to apply a combined patch for patch series. See
@@ -187,7 +187,7 @@ sub Submit
   my $First = 1;
   foreach my $BaseName (keys %Targets)
   {
-    my $Jobs = WineTestBot::Jobs::CreateJobs();
+    my $Jobs = CreateJobs();
 
     # Create a new job for this patch
     my $NewJob = $Jobs->Add();
@@ -283,7 +283,7 @@ sub Submit
   }
   $self->Disposition($Disposition);
 
-  WineTestBot::Jobs::CreateJobs()->Schedule();
+  ScheduleJobs();
 
   return undef;
 }