Fix validation of nested test jobs

3 files changed, 27 insertions, 24 deletions

diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentInstanceSpec.java b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentInstanceSpec.java
index 2c397c95d5b..8e7c255f27a 100644
--- a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentInstanceSpec.java
+++ b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentInstanceSpec.java
@@ -52,7 +52,7 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps {
         this.athenzService = athenzService;
         this.notifications = notifications;
         this.endpoints = List.copyOf(validateEndpoints(endpoints, steps()));
-        validateZones(new HashSet<>(), this);
+        validateZones(new HashSet<>(), new HashSet<>(), this);
         validateEndpoints(steps(), globalServiceId, this.endpoints);
         validateAthenz();
     }
@@ -64,33 +64,32 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps {
      * or if any production test is declared not after its corresponding deployment.
      *
      * @param deployments previously seen deployments
+     * @param tests previously seen tests
      * @param step step whose members to validate
-     * @return all contained tests
      */
-    private static Set<RegionName> validateZones(Set<RegionName> deployments, DeploymentSpec.Step step) {
+    private static void validateZones(Set<RegionName> deployments, Set<RegionName> tests, DeploymentSpec.Step step) {
         if ( ! step.steps().isEmpty()) {
             Set<RegionName> oldDeployments = Set.copyOf(deployments);
-            Set<RegionName> tests = new HashSet<>();
             for (DeploymentSpec.Step nested : step.steps()) {
-                for (RegionName test : validateZones(deployments, nested)) {
-                    if ( ! (step.isOrdered() ? deployments : oldDeployments).contains(test))
-                        throw new IllegalArgumentException("tests for prod." + test + " must be after the corresponding deployment in deployment.xml");
-
-                    if ( ! tests.add(test))
-                        throw new IllegalArgumentException("tests for prod." + test + " arelisted twice in deployment.xml");
-                }
+                Set<RegionName> seenDeployments = new HashSet<>(step.isOrdered() ? deployments : oldDeployments);
+                validateZones(seenDeployments, tests, nested);
+                deployments.addAll(seenDeployments);
             }
-            return tests;
         }
-        if (step.concerns(Environment.prod)) {
-            if (step.isTest())
-                return Set.of(((DeploymentSpec.DeclaredTest) step).region());
-
-            RegionName region = ((DeploymentSpec.DeclaredZone) step).region().get();
-            if ( ! deployments.add(region))
-                throw new IllegalArgumentException("prod." + region + " is listed twice in deployment.xml");
+        else if (step.concerns(Environment.prod)) {
+            if (step.isTest()) {
+                RegionName region = ((DeploymentSpec.DeclaredTest) step).region();
+                if ( ! deployments.contains(region))
+                    throw new IllegalArgumentException("tests for prod." + region + " must be after the corresponding deployment in deployment.xml");
+                if ( ! tests.add(region))
+                    throw new IllegalArgumentException("tests for prod." + region + " are listed twice in deployment.xml");
+            }
+            else {
+                RegionName region = ((DeploymentSpec.DeclaredZone) step).region().get();
+                if ( ! deployments.add(region))
+                    throw new IllegalArgumentException("prod." + region + " is listed twice in deployment.xml");
+            }
         }
-        return Set.of();
     }
 
     /** Validates the endpoints and makes sure default values are respected */
diff --git a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java
index 584664b6ef1..e4f52f573b0 100644
--- a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java
+++ b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java
@@ -516,6 +516,7 @@ public class DeploymentSpecTest {
                 "               <parallel>" +
                 "                  <region active='true' athenz-service='no-service'>ap-northeast-1</region>" +
                 "                  <region active='true'>ap-southeast-2</region>" +
+                "                  <test>aws-us-east-1a</test>" +
                 "               </parallel>" +
                 "            </steps>" +
                 "            <delay hours='3' minutes='30' />" +
@@ -557,14 +558,15 @@ public class DeploymentSpecTest {
         assertEquals(3, secondSerialSteps.size());
         assertEquals("delay PT3H", secondSerialSteps.get(0).toString());
         assertEquals("prod.aws-us-east-1a", secondSerialSteps.get(1).toString());
-        assertEquals("2 parallel steps", secondSerialSteps.get(2).toString());
+        assertEquals("3 parallel steps", secondSerialSteps.get(2).toString());
 
         List<DeploymentSpec.Step> innerParallelSteps = secondSerialSteps.get(2).steps();
-        assertEquals(2, innerParallelSteps.size());
+        assertEquals(3, innerParallelSteps.size());
         assertEquals("prod.ap-northeast-1", innerParallelSteps.get(0).toString());
         assertEquals("no-service", spec.requireInstance("instance").athenzService(Environment.prod, RegionName.from("ap-northeast-1")).get().value());
         assertEquals("prod.ap-southeast-2", innerParallelSteps.get(1).toString());
         assertEquals("in-service", spec.requireInstance("instance").athenzService(Environment.prod, RegionName.from("ap-southeast-2")).get().value());
+        assertEquals("tests for prod.aws-us-east-1a", innerParallelSteps.get(2).toString());
     }
 
     @Test
diff --git a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java
index 13ec7bf1a8b..00f0a1bfa41 100644
--- a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java
+++ b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java
@@ -367,6 +367,7 @@ public class DeploymentSpecWithoutInstanceTest {
                 "            <parallel>" +
                 "               <region active='true' athenz-service='no-service'>ap-northeast-1</region>" +
                 "               <region active='true'>ap-southeast-2</region>" +
+                "               <test>aws-us-east-1a</test>" +
                 "            </parallel>" +
                 "         </steps>" +
                 "         <delay hours='3' minutes='30' />" +
@@ -407,14 +408,15 @@ public class DeploymentSpecWithoutInstanceTest {
         assertEquals(3, secondSerialSteps.size());
         assertEquals("delay PT3H", secondSerialSteps.get(0).toString());
         assertEquals("prod.aws-us-east-1a", secondSerialSteps.get(1).toString());
-        assertEquals("2 parallel steps", secondSerialSteps.get(2).toString());
+        assertEquals("3 parallel steps", secondSerialSteps.get(2).toString());
 
         List<DeploymentSpec.Step> innerParallelSteps = secondSerialSteps.get(2).steps();
-        assertEquals(2, innerParallelSteps.size());
+        assertEquals(3, innerParallelSteps.size());
         assertEquals("prod.ap-northeast-1", innerParallelSteps.get(0).toString());
         assertEquals("no-service", spec.requireInstance("default").athenzService(Environment.prod, RegionName.from("ap-northeast-1")).get().value());
         assertEquals("prod.ap-southeast-2", innerParallelSteps.get(1).toString());
         assertEquals("service", spec.requireInstance("default").athenzService(Environment.prod, RegionName.from("ap-southeast-2")).get().value());
+        assertEquals("tests for prod.aws-us-east-1a", innerParallelSteps.get(2).toString());
     }
 
     @Test