From a9abccb28b220f02e6d7628dffdb4678a399820c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9s=20de=20la=20Pe=C3=B1a?=
 <a.penya.garcia@gmail.com>
Date: Tue, 10 Aug 2021 15:22:49 +0100
Subject: [PATCH] Fix flaky SSTableExportTest to run first the tests calling
 assertSchemaNotLoaded
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

patch by Andrés de la Peña; reviewed by Ekaterina Dimitrova and Berenguer Blasi for CASSANDRA-16818
---
 .../cassandra/tools/SSTableExportTest.java    | 61 +++++++++++--------
 1 file changed, 35 insertions(+), 26 deletions(-)

diff --git a/test/unit/org/apache/cassandra/tools/SSTableExportTest.java b/test/unit/org/apache/cassandra/tools/SSTableExportTest.java
index 460df82a06..eb082dd99b 100644
--- a/test/unit/org/apache/cassandra/tools/SSTableExportTest.java
+++ b/test/unit/org/apache/cassandra/tools/SSTableExportTest.java
@@ -43,7 +43,7 @@ import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
-@RunWith(OrderedJUnit4ClassRunner.class)
+@RunWith(OrderedJUnit4ClassRunner.class) // tests calling assertSchemaNotLoaded should be the first ones
 public class SSTableExportTest extends OfflineToolUtils
 {
     private static final ObjectMapper mapper = new ObjectMapper();
@@ -95,6 +95,28 @@ public class SSTableExportTest extends OfflineToolUtils
         assertPostTestEnv(false);
     }
 
+    @Test
+    public void testPKArgOutOfOrder()
+    {
+        ToolResult tool = ToolRunner.invokeClass(SSTableExport.class, "-k", "0", sstable);
+        assertThat(tool.getStdout(), containsStringIgnoringCase("usage:"));
+        assertThat(tool.getCleanedStderr(), containsStringIgnoringCase("You must supply exactly one sstable"));
+        assertThat(tool.getCleanedStderr(), containsStringIgnoringCase("before the -k/-x options"));
+        assertEquals(1, tool.getExitCode());
+        assertPostTestEnv(false);
+    }
+
+    @Test
+    public void testExcludePKArgOutOfOrder()
+    {
+        ToolResult tool = ToolRunner.invokeClass(SSTableExport.class, "-x", "0", sstable);
+        assertThat(tool.getStdout(), containsStringIgnoringCase("usage:"));
+        assertThat(tool.getCleanedStderr(), containsStringIgnoringCase("You must supply exactly one sstable"));
+        assertThat(tool.getCleanedStderr(), containsStringIgnoringCase("before the -k/-x options"));
+        assertEquals(1, tool.getExitCode());
+        assertPostTestEnv(false);
+    }
+
     @Test
     public void testDefaultCall() throws IOException
     {
@@ -135,17 +157,6 @@ public class SSTableExportTest extends OfflineToolUtils
         assertPostTestEnv(true);
     }
 
-    @Test
-    public void testPKArgOutOfOrder()
-    {
-        ToolResult tool = ToolRunner.invokeClass(SSTableExport.class, "-k", "0", sstable);
-        assertThat(tool.getStdout(), containsStringIgnoringCase("usage:"));
-        assertThat(tool.getCleanedStderr(), containsStringIgnoringCase("You must supply exactly one sstable"));
-        assertThat(tool.getCleanedStderr(), containsStringIgnoringCase("before the -k/-x options"));
-        assertEquals(1, tool.getExitCode());
-        assertPostTestEnv(false);
-    }
-
     @Test
     public void testMultiplePKArg() throws IOException
     {
@@ -162,17 +173,6 @@ public class SSTableExportTest extends OfflineToolUtils
         assertPostTestEnv(true);
     }
 
-    @Test
-    public void testExcludePKArgOutOfOrder()
-    {
-        ToolResult tool = ToolRunner.invokeClass(SSTableExport.class, "-x", "0", sstable);
-        assertThat(tool.getStdout(), containsStringIgnoringCase("usage:"));
-        assertThat(tool.getCleanedStderr(), containsStringIgnoringCase("You must supply exactly one sstable"));
-        assertThat(tool.getCleanedStderr(), containsStringIgnoringCase("before the -k/-x options"));
-        assertEquals(1, tool.getExitCode());
-        assertPostTestEnv(false);
-    }
-
     @Test
     public void testMultipleExcludePKArg() throws IOException
     {
@@ -239,12 +239,21 @@ public class SSTableExportTest extends OfflineToolUtils
         assertPostTestEnv(true);
     }
 
-    private void assertPostTestEnv(boolean loadsSchema)
+    /**
+     * Runs post-test assertions about loaded classed and started threads.
+     *
+     * @param maybeLoadsSchema {@code true} if the test may or may not have loaded the schema depending on the JVM,
+     * {@code false} if the test shoudln't load the schema in any case. Note that a test not loading the schema can
+     * still end with the schema loaded if a previous test already loaded it, so we should always run first the tests
+     * that don't load the schema, and then the ones that may or may not load it. We also need to use the
+     * {@link OrderedJUnit4ClassRunner} runner to guarantee the desired run order.
+     */
+    private void assertPostTestEnv(boolean maybeLoadsSchema)
     {
         assertNoUnexpectedThreadsStarted(null, OPTIONAL_THREADS_WITH_SCHEMA);
-        // schema loading seems to depend of the JVM version,
+        // schema loading seems to depend on the JVM version,
         // so we only verify the cases where we are sure it's not loaded
-        if (!loadsSchema)
+        if (!maybeLoadsSchema)
             assertSchemaNotLoaded();
         assertCLSMNotLoaded();
         assertSystemKSNotLoaded();