[GitHub] [cassandra] michaelsembwever commented on a diff in pull request #1642: CASSANDRA-17656: jvm-dtest upgrade tests run all supported pairs of upgrades between from/to but does not actually test all patches from/to

[GitBox <gi...@apache.org>]

michaelsembwever commented on code in PR #1642:
URL: https://github.com/apache/cassandra/pull/1642#discussion_r886876061


##########
test/distributed/org/apache/cassandra/distributed/upgrade/UpgradeTestBase.java:
##########
@@ -153,20 +184,40 @@ public TestCase upgradesFrom(Semver from)
         /** performs all supported upgrade paths that exist in between from and to (inclusive) **/
         public TestCase upgrades(Semver from, Semver to)
         {
-            SUPPORTED_UPGRADE_PATHS.stream()
-                .filter(upgradePath -> (upgradePath.left.compareTo(from) >= 0 && upgradePath.right.compareTo(to) <= 0))
-                .forEachOrdered(upgradePath ->
+            List<TestVersions> upgrade = new ArrayList<>();
+            List<Version> toVersions = Collections.singletonList(versions.getLatest(to));
+            NavigableSet<Semver> vertices = sortedVertices(SUPPORTED_UPGRADE_PATHS);
+            for (Semver start : vertices.subSet(from, to))
+            {
+                // only include pairs that are allowed
+                if (SUPPORTED_UPGRADE_PATHS.hasEdge(start, to))
+                    upgrade.add(new TestVersions(versions.getLatest(start), toVersions));
+            }
+            if (CURRENT.equals(from))
+            {
+                // when from=CURRENT we want to test upgrading to more recent versions rather than just upgrading
+                // from previous versions; so special case that
+                for (Semver end : vertices.subSet(from, false, to, false)) //from->to included above, so don't need to readd
                 {
-                    this.upgrade.add(
-                            new TestVersions(versions.getLatest(upgradePath.left), versions.getLatest(upgradePath.right)));
-                });
+                    if (SUPPORTED_UPGRADE_PATHS.hasEdge(CURRENT, end))
+                        upgrade.add(new TestVersions(versions.getLatest(CURRENT), Collections.singletonList(versions.getLatest(end))));
+                }
+            }

Review Comment:
   curious if this can be simplified to
   ```
   
               List<TestVersions> upgrade = new ArrayList<>();
               NavigableSet<Semver> vertices = sortedVertices(SUPPORTED_UPGRADE_PATHS);
               for (Semver vertex : vertices.subSet(from, true, to, true))
               {
                   if (SUPPORTED_UPGRADE_PATHS.hasEdge(CURRENT, vertex))
                       upgrade.add(new TestVersions(versions.getLatest(CURRENT), Collections.singletonList(versions.getLatest(vertex))));
                   
                   if (SUPPORTED_UPGRADE_PATHS.hasEdge(vertex, CURRENT))
                       upgrade.add(new TestVersions(versions.getLatest(vertex), Collections.singletonList(versions.getLatest(CURRENT))));
               }
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org