Merge pull request #24029 from vespa-engine/jonmv/avoid-unnecessary-system-tests

Do not require equal source versions for system tests to be equal

2 files changed, 17 insertions, 9 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java
index 6a3a44c0140..3a6a8e67a75 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java
@@ -656,6 +656,7 @@ public class DeploymentStatus {
     /** The test jobs that need to run prior to the given production deployment jobs. */
     public Map<JobId, List<Job>> testJobs(Map<JobId, List<Job>> jobs) {
         Map<JobId, List<Job>> testJobs = new LinkedHashMap<>();
+        // First, look for a declared test in the instance of each production job.
         jobs.forEach((job, versionsList) -> {
             for (JobType testType : List.of(systemTest(job.type()), stagingTest(job.type()))) {
                 if (job.type().isProduction() && job.type().isDeployment()) {
@@ -673,6 +674,7 @@ public class DeploymentStatus {
                 }
             }
         });
+        // If no declared test in the right instance was triggered, pick one from a different instance.
         jobs.forEach((job, versionsList) -> {
             for (JobType testType : List.of(systemTest(job.type()), stagingTest(job.type()))) {
                 for (Job productionJob : versionsList)
@@ -680,7 +682,8 @@ public class DeploymentStatus {
                         && allJobs.successOn(testType, productionJob.versions()).asList().isEmpty()
                         && testJobs.keySet().stream()
                                    .noneMatch(test ->    test.type().equals(testType) && test.type().zone().equals(testType.zone())
-                                                      && testJobs.get(test).stream().anyMatch(testJob -> testJob.versions().equals(productionJob.versions())))) {
+                                                      && testJobs.get(test).stream().anyMatch(testJob -> test.type().isSystemTest() ? testJob.versions().targetsMatch(productionJob.versions())
+                                                                                                                                    : testJob.versions().equals(productionJob.versions())))) {
                         JobId testJob = firstDeclaredOrElseImplicitTest(testType);
                         testJobs.merge(testJob,
                                        List.of(new Job(testJob.type(),
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java
index b533a0c7797..89b6f6ca606 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java
@@ -63,7 +63,6 @@ import static com.yahoo.vespa.hosted.controller.deployment.DeploymentContext.tes
 import static com.yahoo.vespa.hosted.controller.deployment.DeploymentContext.testUsWest1;
 import static com.yahoo.vespa.hosted.controller.deployment.DeploymentTrigger.ChangesToCancel.ALL;
 import static com.yahoo.vespa.hosted.controller.deployment.DeploymentTrigger.ChangesToCancel.PLATFORM;
-import static java.util.Collections.copy;
 import static java.util.Collections.emptyList;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
@@ -2563,12 +2562,18 @@ public class DeploymentTriggerTest {
         assertEquals(Set.of(), tests.deploymentStatus().jobsToRun().keySet());
     }
 
-
     @Test
     void testInstancesWithMultipleClouds() {
         String spec = """
                       <deployment>
                         <parallel>
+                          <instance id='separate'>
+                            <test />
+                            <staging />
+                            <prod>
+                              <region>alpha-centauri</region>
+                            </prod>
+                          </instance>
                           <instance id='independent'>
                             <test />
                           </instance>
@@ -2599,10 +2604,9 @@ public class DeploymentTriggerTest {
                               </prod>
                             </instance>
                           </steps>
-                          <instance id='separate'>
-                            <staging />
+                          <instance id='dependent'>
                             <prod>
-                              <region>alpha-centauri</region>
+                              <region>us-east-3</region>
                             </prod>
                           </instance>
                         </parallel>
@@ -2620,17 +2624,18 @@ public class DeploymentTriggerTest {
         tester.configServer().bootstrap(tester.controllerTester().zoneRegistry().zones().all().ids(), SystemApplication.notController());
 
         ApplicationPackage appPackage = ApplicationPackageBuilder.fromDeploymentXml(spec);
-        DeploymentContext app = tester.newDeploymentContext("tenant", "application", "alpha").submit(appPackage);
+        DeploymentContext app = tester.newDeploymentContext("tenant", "application", "alpha").submit(appPackage).deploy();
+        app.submit(appPackage);
         Map<JobId, List<DeploymentStatus.Job>> jobs = app.deploymentStatus().jobsToRun();
 
         JobType centauriTest = JobType.systemTest(tester.controller().zoneRegistry(), CloudName.from("centauri"));
         JobType centauriStaging = JobType.stagingTest(tester.controller().zoneRegistry(), CloudName.from("centauri"));
+        assertQueued("separate", jobs, centauriTest);
+        assertQueued("separate", jobs, stagingTest, centauriStaging);
         assertQueued("independent", jobs, systemTest, centauriTest);
         assertQueued("alpha", jobs, systemTest);
         assertQueued("beta", jobs, centauriTest);
         assertQueued("gamma", jobs, centauriTest);
-        assertQueued("nu", jobs, stagingTest);
-        assertQueued("separate", jobs, centauriStaging);
 
         // Once alpha runs its default system test, it also runs the centauri system test, as omega depends on it.
         app.runJob(systemTest);