Let DeploymentSteps insert test jobs (do not trust DeploymentSpec)

4 files changed, 7 insertions, 30 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentSteps.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentSteps.java
index 6af592cad37..3b19b8930c8 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentSteps.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentSteps.java
@@ -18,6 +18,7 @@ import java.util.Objects;
 import java.util.Optional;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import static java.util.Comparator.comparingInt;
 import static java.util.stream.Collectors.collectingAndThen;
@@ -39,9 +40,10 @@ public class DeploymentSteps {
 
     /** Returns jobs for this, in the order they are declared */
     public List<JobType> jobs() {
-        return spec.steps().stream()
-                   .flatMap(step -> toJobs(step).stream())
-                   .collect(Collectors.toUnmodifiableList());
+        return Stream.concat(production().isEmpty() ? Stream.of() : Stream.of(JobType.systemTest, JobType.stagingTest),
+                             spec.steps().stream().flatMap(step -> toJobs(step).stream()))
+                     .distinct()
+                     .collect(Collectors.toUnmodifiableList());
     }
 
     /** Returns job status sorted according to deployment spec */
@@ -73,7 +75,7 @@ public class DeploymentSteps {
 
     /** Returns declared test jobs in this */
     public List<JobType> testJobs() {
-        return toJobs(test());
+        return jobs().stream().filter(JobType::isTest).collect(Collectors.toUnmodifiableList());
     }
 
     /** Returns declared production jobs in this */
@@ -81,13 +83,6 @@ public class DeploymentSteps {
         return toJobs(production());
     }
 
-    /** Returns declared test steps in this */
-    public List<DeploymentSpec.Step> test() {
-        return spec.steps().stream()
-                   .filter(step -> isTest(step))
-                   .collect(Collectors.toUnmodifiableList());
-    }
-
     /** Returns declared production steps in this */
     public List<DeploymentSpec.Step> production() {
         return spec.steps().stream()
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
index 3f86d996ad7..ca2db96c14d 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
@@ -579,7 +579,7 @@ public class DeploymentTrigger {
     private List<Job> testJobs(DeploymentSpec deploymentSpec, Change change, Instance instance, Versions versions,
                                String reason, Instant availableSince, Predicate<JobType> condition) {
         List<Job> jobs = new ArrayList<>();
-        for (JobType jobType : List.of(systemTest, stagingTest)) { // TODO jonmv: Allow cross-instance validation
+        for (JobType jobType : new DeploymentSteps(deploymentSpec.requireInstance(instance.name()), controller::system).testJobs()) { // TODO jonmv: Allow cross-instance validation
             Optional<JobRun> completion = successOn(instance, jobType, versions)
                     .filter(run -> versions.sourcesMatchIfPresent(run) || jobType == systemTest);
             if (completion.isEmpty() && condition.test(jobType))
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
index f91f35a6b1b..c352fc5550f 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
@@ -46,8 +46,6 @@ public class ApplicationPackageBuilder {
     private OptionalInt majorVersion = OptionalInt.empty();
     private String instances = "default";
     private String upgradePolicy = null;
-    private boolean explicitSystemTest = false;
-    private boolean explicitStagingTest = false;
     private Environment environment = Environment.prod;
     private String globalServiceId = null;
     private String athenzIdentityAttributes = null;
@@ -63,16 +61,6 @@ public class ApplicationPackageBuilder {
         return this;
     }
 
-    public ApplicationPackageBuilder systemTest() {
-        this.explicitSystemTest = true;
-        return this;
-    }
-
-    public ApplicationPackageBuilder stagingTest() {
-        this.explicitStagingTest = true;
-        return this;
-    }
-
     public ApplicationPackageBuilder upgradePolicy(String upgradePolicy) {
         this.upgradePolicy = upgradePolicy;
         return this;
@@ -189,10 +177,6 @@ public class ApplicationPackageBuilder {
         }
         xml.append(notifications);
         xml.append(blockChange);
-        if (explicitSystemTest)
-            xml.append("    <test />\n");
-        if (explicitStagingTest)
-            xml.append("    <staging />\n");
         xml.append("    <");
         xml.append(environment.value());
         if (globalServiceId != null) {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
index 417842fb608..5deedaa2fb1 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
@@ -664,8 +664,6 @@ public class ApplicationApiTest extends ControllerContainerTest {
         // Sixth attempt has a multi-instance deployment spec, and is accepted.
         ApplicationPackage multiInstanceSpec = new ApplicationPackageBuilder()
                 .instances("instance1,instance2")
-                .systemTest()
-                .stagingTest()
                 .environment(Environment.prod)
                 .region("us-central-1")
                 .parallel("us-west-1", "us-east-3")