New subject: [PATCH 2/2] testbot/Submit: Add a single page mode.

2 Oct 2019

Patches submitted through the submit page no longer need to impact one
and only one test unit. This means it's no longer necessary to split
longer patches in order to test them.
With the Wine VMs it also no longer makes sense to ban patches that
don't touch the tests as one may need to make sure they compile and
don't cause regressions in existing tests.
However it is still necessary to specify command line arguments for
the test executable which means picking a single test unit to run.

Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com>
---
 testbot/bin/WineRunWineTest.pl        |   1 +
 testbot/bin/build/Build.pl            |   6 +
 testbot/bin/build/WineTest.pl         |  79 +++++++++----
 testbot/lib/Build/Utils.pm            |  43 +++++---
 testbot/lib/WineTestBot/PatchUtils.pm |   1 +
 testbot/web/Submit.pl                 | 153 ++++++++++++++++++--------
 6 files changed, 199 insertions(+), 84 deletions(-)

diff --git a/testbot/bin/WineRunWineTest.pl b/testbot/bin/WineRunWineTest.pl
index beb050276..ff5317c8b 100755
--- a/testbot/bin/WineRunWineTest.pl
+++ b/testbot/bin/WineRunWineTest.pl
@@ -484,6 +484,7 @@ if ($Step->Type eq "suite")
 elsif ($Step->FileType eq "patch")
 {
   $Script .= "--testpatch ". ShQuote($Task->Missions) ." patch.diff";
+  $Script .= " ". $Task->CmdLineArg if (defined $Task->CmdLineArg);
 }
 else
 {
diff --git a/testbot/bin/build/Build.pl b/testbot/bin/build/Build.pl
index f23b733d6..4c342a5ac 100755
--- a/testbot/bin/build/Build.pl
+++ b/testbot/bin/build/Build.pl
@@ -87,6 +87,12 @@ sub BuildTestExecutables($$$)
   }
 
   InfoMsg "\nBuilding the $Build Wine test executable(s)\n";
+  if (!@BuildDirs)
+  {
+    InfoMsg "Nothing to do\n";
+    return 1;
+  }
+
   my $CPUCount = GetCPUCount();
   system("cd '$DataDir/wine-$Build' && set -x && ".
          "time make -j$CPUCount ". join(" ", sort @BuildDirs));
diff --git a/testbot/bin/build/WineTest.pl b/testbot/bin/build/WineTest.pl
index 37c20d1b4..9948e0929 100755
--- a/testbot/bin/build/WineTest.pl
+++ b/testbot/bin/build/WineTest.pl
@@ -121,9 +121,7 @@ sub DailyWineTest($$$$)
 
 sub TestPatch($$)
 {
-  my ($Mission, $Impacts) = @_;
-
-  return 1 if ($Mission->{test} eq "build");
+  my ($Mission, $Impacts, $Args) = @_;
 
   my @TestList;
   if ($Mission->{test} eq "all" and
@@ -180,6 +178,24 @@ sub TestPatch($$)
   return 1;
 }
 
+sub GetExecutable($$$)
+{
+  my ($Mission, $Impacts, $Module) = @_;
+
+  my $TestInfo = $Impacts->{Tests}->{$Module};
+  my $TestExecutable = "$TestInfo->{ExeBase}.exe";
+  my $Dst = "$DataDir/staging/$TestExecutable";
+  unlink $Dst;
+
+  my $WineDir = GetWineDir($Mission);
+  if (!symlink "$WineDir/$TestInfo->{Path}/$TestExecutable", $Dst)
+  {
+    LogMsg "Could not find $TestExecutable: $!\n";
+    return undef;
+  }
+  return $TestExecutable;
+}
+
 sub TestExe($$$)
 {
   my ($Mission, $FileName, $Args) = @_;
@@ -196,6 +212,21 @@ sub TestExe($$$)
     }
   }
 
+  my $TestDir = GetTestDir($Mission);
+  if ($TestDir eq ".")
+  {
+    $FileName = "$DataDir/staging/$FileName";
+  }
+  else
+  {
+    unlink "$TestDir/$FileName";
+    if (!symlink "$DataDir/staging/$FileName", "$TestDir/$FileName")
+    {
+      LogMsg "Could not create '$FileName' in the test directory: $!\n";
+      return 0;
+    }
+  }
+
   # Run the test executable
   RunWine($Mission, GetTestLauncher($Mission),
           "-t 120 ". ShArgv2Cmd($FileName, @$Args)
@@ -210,7 +241,7 @@ sub TestExe($$$)
 #
 
 my $Action = "";
-my ($Usage, $OptNoSubmit, $MissionStatement, $FileName, $BaseTag);
+my ($Usage, $OptNoSubmit, $MissionStatement, $FileName, $Module, $BaseTag);
 while (@ARGV)
 {
   my $Arg = shift @ARGV;
@@ -248,17 +279,16 @@ while (@ARGV)
   elsif ($Action eq "winetest")
   {
     $BaseTag = $Arg;
-    # The remaining arguments are meant for WineTest
-    last;
+    last; # The remaining arguments are meant for WineTest
   }
   elsif (!defined $FileName)
   {
     if (IsValidFileName($Arg))
     {
-      $FileName = "$DataDir/staging/$Arg";
-      if (!-r $FileName)
+      $FileName = $Arg;
+      if (!-r "$DataDir/staging/$FileName")
       {
-        Error "'$Arg' is not readable\n";
+        Error "'$FileName' is not readable\n";
         $Usage = 2;
       }
     }
@@ -266,14 +296,17 @@ while (@ARGV)
     {
       Error "the '$Arg' filename contains invalid characters\n";
       $Usage = 2;
-      last;
     }
     if ($Action eq "testexe")
     {
-      # The remainder are the executable's arguments
-      last;
+      last; # The remainder are the executable's arguments
     }
   }
+  elsif ($Action eq "testpatch" and !defined $Module)
+  {
+    $Module = $Arg;
+    last; # The remainder are the executable's arguments
+  }
   else
   {
     Error "unexpected argument '$Arg'\n";
@@ -372,7 +405,7 @@ if (defined $Usage)
     exit $Usage;
   }
   print "Usage: $Name0 [--help] --testexe MISSIONS EXE ARGS...\n";
-  print "or     $Name0 [--help] --testpatch MISSIONS PATCH\n";
+  print "or     $Name0 [--help] --testpatch MISSIONS PATCH [MODULE ARGS...]\n";
   print "or     $Name0 [--help] --winetest [--no-submit] MISSIONS BASETAG ARGS\n";
   print "\n";
   print "Tests the specified patch or runs WineTest in Wine.\n";
@@ -387,11 +420,10 @@ if (defined $Usage)
   print "               - wow32: The 32 bit WoW Wine build.\n";
   print "               - wow64: The 64 bit WoW Wine build.\n";
   print "  EXE          Is the staging file containing the executable to run.\n";
-  print "  ARGS         Are the arguments for the test executable.\n";
   print "  PATCH        Is the staging file containing the patch to test.\n";
   print "  BASETAG      Is the tag for this WineTest run. Note that the build type is\n";
   print "               automatically added to this tag.\n";
-  print "  ARGS         The WineTest arguments.\n";
+  print "  ARGS         Arguments for the test.\n";
   print "  --help       Shows this usage message.\n";
   exit $Usage;
 }
@@ -413,7 +445,7 @@ unlink map { GetMissionBaseName($_) .".report" } @{$TaskMissions->{Missions}};
 my $Impacts;
 if ($Action eq "testpatch")
 {
-  $Impacts = ApplyPatch("wine", $FileName);
+  $Impacts = ApplyPatch("wine", "$DataDir/staging/$FileName");
   exit(1) if (!$Impacts or
               !BuildWine($TaskMissions, "win32") or
               !BuildWine($TaskMissions, "wow64") or
@@ -421,18 +453,25 @@ if ($Action eq "testpatch")
 }
 foreach my $Mission (@{$TaskMissions->{Missions}})
 {
+  return 1 if ($Mission->{test} eq "build");
+
   if ($Action eq "testexe")
   {
     exit(1) if (!TestExe($Mission, $FileName, \@ARGV));
   }
-  elsif ($Action eq "testpatch")
-  {
-    exit(1) if (!TestPatch($Mission, $Impacts));
-  }
   elsif ($Action eq "winetest")
   {
     exit(1) if (!DailyWineTest($Mission,  $OptNoSubmit, $BaseTag, \@ARGV));
   }
+  elsif (@ARGV)
+  {
+    my $FileName = GetExecutable($Mission, $Impacts, $Module);
+    exit(1) if (!$FileName or !TestExe($Mission, $FileName, \@ARGV));
+  }
+  else
+  {
+    exit(1) if (!TestPatch($Mission, $Impacts));
+  }
 }
 
 LogMsg "ok\n";
diff --git a/testbot/lib/Build/Utils.pm b/testbot/lib/Build/Utils.pm
index 50146301a..15acf6783 100644
--- a/testbot/lib/Build/Utils.pm
+++ b/testbot/lib/Build/Utils.pm
@@ -30,7 +30,8 @@ our @EXPORT = qw(GetToolName InfoMsg LogMsg Error
                  GitPull ApplyPatch
                  GetCPUCount BuildNativeTestAgentd BuildWindowsTestAgentd
                  GetTestLauncher BuildTestLauncher UpdateAddOns
-                 SetupWineEnvironment RunWine CreateWinePrefix);
+                 SetupWineEnvironment GetWineDir GetTestDir RunWine
+                 CreateWinePrefix);
 
 use Digest::SHA;
 use File::Basename;
@@ -397,29 +398,37 @@ sub SetupWineEnvironment($)
   return $BaseName;
 }
 
+sub GetWineDir($)
+{
+  my ($Mission) = @_;
+  return "$DataDir/wine-$Mission->{Build}";
+}
+
+sub GetTestDir($)
+{
+  my ($Mission) = @_;
+
+  my $DevDir = "$ENV{WINEPREFIX}/dosdevices";
+  return "." if (!-d $DevDir);
+
+  my $Dir = $Mission->{dir} || "";
+  # We cannot put colons in missions so restore the drive letter
+  $Dir = "$DevDir/c:/$Dir" if ($Dir !~ s~^([a-z])/~$DevDir/$1:/~);
+  return -d $Dir ? $Dir : "$DevDir/c:";
+}
+
 sub RunWine($$$)
 {
   my ($Mission, $Cmd, $CmdArgs) = @_;
 
-  my $WineDir = "$DataDir/wine-$Mission->{Build}";
-  $Cmd = "$WineDir/$Cmd" if ($Cmd =~ /\.exe\.so$/);
-
-  my $CurDir = "$ENV{WINEPREFIX}/dosdevices";
-  if (!-d $CurDir)
-  {
-    $CurDir = ".";
-  }
-  else
-  {
-    my $Dir = $Mission->{dir} || "";
-    # We cannot put colons in missions so restore the drive letter
-    $Dir = "$CurDir/c:/$Dir" if ($Dir !~ s~^([a-z])/~$CurDir/$1:/~);
-    $CurDir = -d $Dir ? $Dir : "$ENV{WINEPREFIX}/dosdevices/c:";
-  }
+  my $TestDir = GetTestDir($Mission);
 
+  my $WineDir = GetWineDir($Mission);
+  $Cmd = "$WineDir/$Cmd" if ($Cmd =~ /\.exe\.so$/);
   my $Magic = `file '$Cmd'`;
   my $Wine = ($Magic =~ /ELF 64/ ? "$WineDir/wine64" : "$WineDir/wine");
-  return system("set -x && cd '$CurDir' && ".
+
+  return system("set -x && cd '$TestDir' && ".
                 "time '$Wine' '$Cmd' $CmdArgs");
 }
 
diff --git a/testbot/lib/WineTestBot/PatchUtils.pm b/testbot/lib/WineTestBot/PatchUtils.pm
index 71be9cf00..3c2f67011 100644
--- a/testbot/lib/WineTestBot/PatchUtils.pm
+++ b/testbot/lib/WineTestBot/PatchUtils.pm
@@ -441,6 +441,7 @@ sub GetPatchImpacts($)
       next if (exists $TestInfo->{Files}->{"$Base.spec"});
       # Don't try running a deleted test unit obviously
       next if ($TestInfo->{Files}->{$File} eq "rm");
+      $TestInfo->{AllUnits}->{$Base} = 1;
 
       if ($TestInfo->{All} or $TestInfo->{Files}->{$File})
       {
diff --git a/testbot/web/Submit.pl b/testbot/web/Submit.pl
index a51ace650..b67237528 100644
--- a/testbot/web/Submit.pl
+++ b/testbot/web/Submit.pl
@@ -32,6 +32,7 @@ use POSIX qw(:fcntl_h); # For SEEK_XXX
 use File::Basename;
 
 use ObjectModel::BasicPropertyDescriptor;
+use ObjectModel::EnumPropertyDescriptor;
 use WineTestBot::Branches;
 use WineTestBot::Config;
 use WineTestBot::Engine::Notify;
@@ -174,16 +175,10 @@ sub _AnalyzePatch($)
     $self->{ErrMessage} = "'$self->{FileName}' is not a valid patch";
     return undef;
   }
-  if ($self->{Impacts}->{TestUnitCount} == 0)
+  if ($self->{Impacts}->{ModuleUnitCount} == 0)
   {
     $self->{ErrField} = "FileName";
-    $self->{ErrMessage} = "The patch does not impact the tests";
-    return undef;
-  }
-  if ($self->{Impacts}->{TestUnitCount} > 1)
-  {
-    $self->{ErrField} = "FileName";
-    $self->{ErrMessage} = "The patch contains changes to multiple tests";
+    $self->{ErrMessage} = "The patch does not directly impact the Wine dlls and programs";
     return undef;
   }
   return 1;
@@ -271,6 +266,65 @@ sub _ValidateVMSelection($;$)
   return 1;
 }
 
+sub _GetTestExecutables($)
+{
+  my ($self, $ResetInvalid) = @_;
+
+  if (!$self->{TestExecutables})
+  {
+    if ($self->{FileType} eq "patch")
+    {
+      foreach my $Module (sort keys %{$self->{Impacts}->{Tests}})
+      {
+        my $TestInfo = $self->{Impacts}->{Tests}->{$Module};
+        next if (!%{$TestInfo->{Files}});
+        $self->{TestExecutables}->{"$TestInfo->{ExeBase}.exe"} = $Module;
+      }
+    }
+    else
+    {
+      $self->{TestExecutables}->{$self->{FileName}} = 1;
+    }
+  }
+}
+
+sub _ValidateTestExecutable($;$)
+{
+  my ($self, $ResetInvalid) = @_;
+
+  $self->_GetTestExecutables();
+  if ($ResetInvalid and (!defined $self->{TestExecutable} or
+                         !$self->{TestExecutables}->{$self->{TestExecutable}}))
+  {
+    $self->{TestExecutable} = (sort keys %{$self->{TestExecutables}})[0];
+    if (!defined $self->{CmdLineArg} and $self->{Impacts})
+    {
+      my $Module = $self->{TestExecutables}->{$self->{TestExecutable}};
+      my $TestInfo = $self->{Impacts}->{Tests}->{$Module};
+      $self->{CmdLineArg} = (sort keys %{$TestInfo->{PatchedUnits}})[0] ||
+                            (sort keys %{$TestInfo->{AllUnits}})[0];
+    }
+  }
+
+  if (!defined $self->{TestExecutable})
+  {
+    $self->{ErrField} = "TestExecutable";
+    $self->{ErrMessage} = "You must specify the test executable filename";
+    return undef;
+  }
+  elsif (!$self->{TestExecutables}->{$self->{TestExecutable}} or
+         # Make sure the test executable filename is valid
+         # to defend against malicious patches.
+         !IsValidFileName($self->{TestExecutable}))
+  {
+    $self->{ErrField} = "TestExecutable";
+    $self->{ErrMessage} = "Invalid test executable filename";
+    return undef;
+  }
+
+  return 1;
+}
+
 sub Validate($)
 {
   my ($self) = @_;
@@ -318,14 +372,7 @@ sub Validate($)
 
   if ($self->{Page} >= 3)
   {
-    if (($self->{FileType} eq "patch" and
-         $self->{TestExecutable} !~ m/^[\w_.]+_test\.exe$/) or
-        !IsValidFileName($self->{TestExecutable}))
-    {
-      $self->{ErrField} = "TestExecutable";
-      $self->{ErrMessage} = "Invalid test executable filename";
-      return undef;
-    }
+    return undef if (!$self->_ValidateTestExecutable());
 
     if ($self->{CmdLineArg} eq "" and !$self->{NoCmdLineArgWarn})
     {
@@ -493,6 +540,30 @@ sub GetHeaderText($)
     }
     return $HeaderText;
   }
+  elsif ($self->{Page} == 3 and $self->{Impacts})
+  {
+    my @TestExecutables;
+    $self->_GetTestExecutables();
+    foreach my $TestExecutable (sort keys %{$self->{TestExecutables}})
+    {
+      my $Module = $self->{TestExecutables}->{$TestExecutable};
+      my $TestInfo = $self->{Impacts}->{Tests}->{$Module};
+      my @TestUnits;
+      foreach my $TestUnit (sort keys %{$TestInfo->{AllUnits}})
+      {
+        if ($TestInfo->{PatchedUnits}->{$TestUnit})
+        {
+          push @TestUnits, "<i>$TestUnit</i>";
+        }
+        elsif ($TestInfo->{All} or $TestInfo->{PatchedModule})
+        {
+          push @TestUnits, $TestUnit;
+        }
+      }
+      push @TestExecutables, "$Module: ". join(" ", @TestUnits) if (@TestUnits);
+    }
+    return join("<br>\n", "Here is a list of the test executables impacted by the patch (patched test units, if any, are in italics).", @TestExecutables);
+  }
 
   return "";
 }
@@ -512,15 +583,18 @@ sub GetPropertyDescriptors($)
   }
   elsif ($self->{Page} == 3)
   {
-    $self->_GetFileType();
-    my $IsPatch = ($self->{FileType} eq "patch");
-    $self->{PropertyDescriptors} ||= [
-      CreateBasicPropertyDescriptor("TestExecutable", "Test executable", !1, $IsPatch, "A", 50),
-      CreateBasicPropertyDescriptor("CmdLineArg", "Command line arguments", !1, !1, "A", 50),
-      CreateBasicPropertyDescriptor("Run64", "Run 64-bit tests in addition to 32-bit tests", !1, 1, "B", 1),
-      CreateBasicPropertyDescriptor("DebugLevel", "Debug level (WINETEST_DEBUG)", !1, 1, "N", 2),
-      CreateBasicPropertyDescriptor("ReportSuccessfulTests", "Report successful tests (WINETEST_REPORT_SUCCESS)", !1, 1, "B", 1),
-    ];
+    if (!$self->{PropertyDescriptors})
+    {
+      $self->_GetTestExecutables();
+      my @TestExecutables = sort keys %{$self->{TestExecutables}};
+      $self->{PropertyDescriptors} = [
+        CreateEnumPropertyDescriptor("TestExecutable", "Test executable", !1, 1, \@TestExecutables),
+        CreateBasicPropertyDescriptor("CmdLineArg", "Command line arguments", !1, !1, "A", 50),
+        CreateBasicPropertyDescriptor("Run64", "Run 64-bit tests in addition to 32-bit tests", !1, 1, "B", 1),
+        CreateBasicPropertyDescriptor("DebugLevel", "Debug level (WINETEST_DEBUG)", !1, 1, "N", 2),
+        CreateBasicPropertyDescriptor("ReportSuccessfulTests", "Report successful tests (WINETEST_REPORT_SUCCESS)", !1, 1, "B", 1),
+      ];
+    }
   }
   return $self->SUPER::GetPropertyDescriptors();
 }
@@ -601,7 +675,9 @@ sub GenerateFields($)
 
       # By default select the base VMs that are ready to run tasks
       if (!$self->{UserVMSelection} and !$VMRow->{Extra} and
-          $VMRow->{VM}->Status !~ /^(?:offline|maintenance)$/)
+          $VMRow->{VM}->Status !~ /^(?:offline|maintenance)$/ and
+          ($VMRow->{VM}->Type eq "wine" or !$self->{Impacts} or
+           $self->{Impacts}->{TestUnitCount}))
       {
         $VMRow->{Checked} = 1;
       }
@@ -764,8 +840,6 @@ sub OnUnset($)
     unlink($StagingFilePath) if ($StagingFilePath);
     delete $self->{FileName};
   }
-  delete $self->{TestExecutable};
-  delete $self->{CmdLineArg};
 
   return 1;
 }
@@ -783,29 +857,13 @@ sub OnPage1Next($)
     $self->{FileName} = $self->GetParam("Upload");
     return undef if (!$self->_ValidateFileName() or !$self->_Upload());
   }
-  if (!$self->Validate() or !$self->_ValidateVMSelection("deselect"))
+  if (!$self->Validate() or !$self->_ValidateVMSelection("deselect") or
+      !$self->_ValidateTestExecutable("reset"))
   {
     return undef;
   }
 
   # Set defaults
-  if ((!defined $self->{TestExecutable} or !defined $self->{CmdLineArg}) and
-      $self->{Impacts})
-  {
-    foreach my $TestInfo (values %{$self->{Impacts}->{Tests}})
-    {
-      next if (!$TestInfo->{UnitCount});
-      if (!defined $self->{TestExecutable})
-      {
-        $self->{TestExecutable} = "$TestInfo->{ExeBase}.exe";
-      }
-      if (!defined $self->{CmdLineArg})
-      {
-        $self->{CmdLineArg} = (keys %{$TestInfo->{PatchedUnits}})[0];
-      }
-      last;
-    }
-  }
   if (!defined $self->{Run64})
   {
     # Whether we show it or not the default is true
@@ -997,7 +1055,8 @@ sub _SubmitJob($$)
     $Task->VM($VM);
     $Task->Timeout($Timeout);
     $Task->Missions($MissionStatement);
-    $Task->CmdLineArg($self->{CmdLineArg}) if ($self->{FileType} ne "patch");
+    my $Module = $self->{TestExecutables}->{$self->{TestExecutable}};
+    $Task->CmdLineArg(($Module eq 1 ? "" : "$Module ") .$self->{CmdLineArg});
   }
 
   # Now save it all (or whatever's left to save)
-- 
2.20.1

    

[PATCH 1/2] testbot/Submit: Allow submitting patches that touch zero or multiple test units.

Francois Gouget

Francois Gouget

tags

participants (1)