Shared mini test suite minor follow on #5555

kevinrr888 · 2025-05-16T16:43:43Z

Brief background: #5536 added a suite to run many of the SharedMiniClusterBase ITs against a single, shared cluster.

The test suite currently includes the ITs which do not set a custom config prior to start up (i.e., those that call SharedMiniClusterBase.startMiniCluster()). This PR was originally going to add the shared mini ITs which set a custom config to the new test suite (i.e., those that call SharedMiniClusterBase.startMiniClusterWithConfig()). However, almost all of the tests which start with a config (except for ~5) change something about server configuration which is needed before starting the servers (e.g., setting the number of servers, setting max memory, changing the class used for the server, etc.). I could not come up with a way to include these in the suite without adding complexity to the ITs (e.g., need to keep track of what servers are configured differently from standard, how many "extra" servers are created, etc.). On top of that, we would still need to restart affected servers after the IT completes. I felt this complexity and the fact restarts were still needed, it was not worth trying to add these to the suite.

So, this PR just adds some trivial improvements that I made when working on this:

Several ITs were not calling stopMiniCluster(). All tests which start a mini cluster now stop the mini cluster as well.
Added some javadoc changes
Misc. other minor changes

kevinrr888 · 2025-05-16T16:46:02Z

test/src/main/java/org/apache/accumulo/test/compaction/FlakyExternalCompaction2IT.java

-
-  @AfterAll
-  public static void teardown() {
-    SharedMiniClusterBase.stopMiniCluster();
-  }


super class already has this AfterAll. Just removed since not needed twice

- Noticed several SharedMiniClusterBase ITs were not calling `stopMiniCluster()`. All tests which start a mini cluster now stop the mini cluster as well. - Added some javadoc changes to SharedMiniClusterBase and SimpleSharedMacTestSuite - Misc. other minor changes

dlmarion · 2025-05-19T12:52:23Z

@kevinrr888 - do you want to try and resolve @ctubbsii's comment as part of this PR? If not, I can take a look at it in a different one.

kevinrr888 · 2025-05-19T14:17:57Z

@dlmarion - I can look into adding that in this PR

ctubbsii · 2025-05-19T19:10:05Z

test/src/main/java/org/apache/accumulo/harness/SharedMiniClusterBase.java

-      // subsequent tests that count objects or initiate
-      // compactions and wait for them, but some other table
-      // from a prior test is compacting.
+      // If stop is disabled, then we are likely running a test class that is part of a larger


I wonder if this should also reset the system config (and any table config affecting accumulo.* tables)?

ctubbsii · 2025-05-19T20:21:41Z

@kevinrr888 - do you want to try and resolve @ctubbsii's comment as part of this PR? If not, I can take a look at it in a different one.

I took a look at this today, because it got in the way of running an explicit test I tried to run with -Dit.test=, and spent some time trying to think about how it could be fixed. Right now, it explicitly overrides the test variable, which is a bit overloaded between the surefire and failsafe plugin. In addition, it also abides by the groups, excludedGroups, forkCount, and other properties that are intended for the other execution. It also conflicts with the -Psunny profile, which activates a group. It gets really complicated to try to write a profile that excludes this second execution whenever any of these options are present. It might be possible if people used the property names only, and then we can have a big list of properties like:

  <profile>
    <id>shared-mini-suite-its</id>
    <activation>
      <property>
        <name>!test</name>
      </property>
      <property>
        <name>!it.test</name>
      </property>
      <property>
        <name>!groups</name>
      </property>
      <property>
        <name>!excludedGroups</name>
      </property>
      <!-- etc.... -->
    </activation>

However, this won't work for the sunny profile, because the profile activation checks run before the sunny profile can set the SunnyDay groups property.

A better option would be to set the second execution in a separate Maven module by moving all these tests to a different module. That's a bit frustrating, and comes with its own downsides, but it would allow us to honor user-specified command-line options by making the plugin behavior driven by properties, rather than hard-coded values that affect the plugin.

An alternative would be to create a suite of new properties that are specific to this execution, and hard-code them into the execution's config, as in:

  <execution>
    <configuration>
      <test>${sharedMiniTest}</test>
      <groups>${sharedMiniGroups}</groups>
      <excludedGroups>${sharedMiniExcludedGroups}</excludedGroups>
      <!-- etc... -->

Another option is to have only the suite entry point test be available for selection, and don't treat it like a separate group. That would require renaming all the ITs in that suite, so they don't get picked up by the *IT naming pattern, and then they really only can be run all or nothing (by executing the suite test class).

None of these seem like great options to me, but something needs to be done to make it possible to run individual tests again without picking up all the suite tests, because they are hard-coded to run every time, regardless of user input. Because of how disruptive it is to not be able to run individual tests, I'm considering reverting the commit that added the suite, unless we can get some workable solution soon. The time savings in Jenkins is not substantial enough to warrant that loss of functionality, in my opinion.

- Removed failsafe plugin from <build><plugins> - Replaced with two profiles: One which runs the specific ITs provoided via it.test variable. One which runs all ITs: first the suite then all other tests excluding the suite. This adds support to running specific ITs via it.test without running the entire suite, which was not the case before. This also allows running specific ITs that are part of the suite without them being excluded.

kevinrr888 · 2025-05-19T21:03:27Z

@ctubbsii - I just pushed what I thought might be a fix for this issue before seeing your comment... Looking over your comment now.

kevinrr888 · 2025-05-20T14:09:02Z

@ctubbsii - What are your thoughts on 8d0be6d? Maybe this meets some of the expectations and can be altered to meet all. With this, can run individual ITs (including those that are part of the suite) without running any other tests. As for the problem you mentioned with -Psunny, maybe this wasn't the case before but with the newest commit, if you run -Psunny, it will execute run-integration-tests-suite which runs tests that are suite && sunny then run-integration-tests-non-suite which runs tests that are !suite && sunny. It doesn't run all of the suite tests. Maybe I'm misunderstanding and you were referring to a different problem, though.

I'm sure my approach has problems, as the Accumulo build and maven in general is fairly new to me.

If you have an alternative approach that you would like to see, please feel free to push a commit.

ctubbsii · 2025-05-22T18:59:50Z

@ctubbsii - What are your thoughts on 8d0be6d? Maybe this meets some of the expectations and can be altered to meet all.

That addresses some things. However, the second execution should not be configured in the parent POM at all. It should only exist in the test module, since that's where the second execution is occurring (most modules do not need a second execution). Also, even with these changes, it will still run when specifying a subset of tests a different way (using -Psunny, -Dfailsafe.groups=, or -Dfailsafe.excludedGroups=, etc.), so having a profile that matches on it.test only is not going to be sufficient. The configuration should not be hard-coded with values, but should allow for the use of properties instead.

I think a simpler approach would be rename the tests included in the suite, so they automatically get skipped by default, and the suite itself is still named ending in IT, so it gets included by default. So the suite is all-or-nothing, but not in a separate execution, but in the same normal execution where you can still specify individual tests (even if they don't follow the naming convention) by using it.test, and all other properties, like groups should work just fine.

ctubbsii · 2025-05-22T19:58:35Z

test/src/main/java/org/apache/accumulo/test/compaction/ExternalCompaction_1_IT.java

@@ -139,6 +139,7 @@ public static void afterTests() throws Exception {
    if (testLock != null) {
      testLock.unlock();
    }
+    stopMiniCluster();


This might need to be backported to 2.1

ctubbsii · 2025-05-22T19:58:52Z

test/src/main/java/org/apache/accumulo/test/compaction/ExternalCompaction_3_IT.java

+  @AfterAll
+  public static void afterTests() {
+    stopMiniCluster();
+  }
+


This might need to be backported to 2.1

ctubbsii · 2025-05-22T19:59:52Z

test/src/main/java/org/apache/accumulo/test/functional/ManagerAssignmentIT.java

@@ -110,6 +110,7 @@ public static void beforeAll() throws Exception {
  @AfterAll
  public static void afterAll() {
    client.close();
+    SharedMiniClusterBase.stopMiniCluster();


This might need to be backported to 2.1

ctubbsii · 2025-05-22T19:59:57Z

test/src/main/java/org/apache/accumulo/test/functional/OnDemandTabletUnloadingIT.java

@@ -116,6 +116,7 @@ public static void beforeAll() throws Exception {

  @AfterAll
  public static void after() throws Exception {
+    SharedMiniClusterBase.stopMiniCluster();


This might need to be backported to 2.1

ctubbsii · 2025-05-22T20:24:17Z

On a phone call with @kevinrr888 , we discussed the following plan, that I think will satisfy everything we want out of this.
In summary:

Check to see if any of the stop minicluster steps need to be backported to 2.1 to prevent leftover processes running in the background of 2.1 builds
Strip out the profile changes in this PR, just to minimize the changes needed for this PR, and merge it with the various javadoc, typo, and other small fixes
Create a new PR to do the following things:
a. Revert the changes to the POM for the failsafe plugin to prior to Shared mini test suite #5536 (so, no second execution or extra config for the suite test), keeping the use of the new dependencies for the suite
b. Rename the SimpleSharedMacTestSuite to SimpleSharedMacTestSuiteIT so it gets picked up by the normal execution of the ITs
c. Rename the ITs included in the suite so end with ITsimpleSuite (or similar) and use that name pattern for the suite
d. Remove the new tag off all the ITs included in the suite, since those aren't needed anymore

I think this gets us everything we want, such as:

By default, everything in the suite will run in the normal build
No messy POM changes that make it hard to override plugin properties on the command-line to execute a specific test
Executing a specific IT can still be done with -Dit.test= to execute a test inside the suite (since the name, when specifying it explicitly, does not need to match the normal pattern)
The suite itself can be run by itself, or excluded, with a pattern that includes or excludes SimpleSharedMacTestSuiteIT

I also think that if this works out well, that we could do something similar with ComprehensiveIT, so it doesn't need to be all contained in the same file, but could be split up for organizational purposes, and ComprehensiveIT could be a suite.

ctubbsii · 2025-05-22T20:28:04Z

The only real downsides I can think of for the suite approach is the inconvenience of having a custom naming conventions for the included suite test cases, and the fact that automatically rerunning a failed flaky test inside the suite with the -Dfailsafe.rerunFailingTestsCount=n option requires rerunning the full suite, since the full suite will fail as a single IT.

ctubbsii

While this didn't accomplish the original goal, that's okay, I think the javadoc and typo fixes, and dependency scope change, are all good fixes, as are the stopping the unstopped mini instances at the end of tests (but some of those probably need to be backported to 2.1).

kevinrr888 added this to the 4.0.0 milestone May 16, 2025

kevinrr888 requested a review from dlmarion May 16, 2025 16:43

kevinrr888 self-assigned this May 16, 2025

kevinrr888 commented May 16, 2025

View reviewed changes

kevinrr888 force-pushed the shared-mini-test-suite-follow-on branch from 36d2bec to 55cbd08 Compare May 16, 2025 17:03

keith-turner approved these changes May 16, 2025

View reviewed changes

dlmarion approved these changes May 19, 2025

View reviewed changes

ctubbsii reviewed May 19, 2025

View reviewed changes

ctubbsii reviewed May 22, 2025

View reviewed changes

revert root pom changes

5825c00

ctubbsii approved these changes May 22, 2025

View reviewed changes

kevinrr888 merged commit 310e2f4 into apache:main May 22, 2025
8 checks passed

kevinrr888 deleted the shared-mini-test-suite-follow-on branch May 22, 2025 20:42

kevinrr888 mentioned this pull request May 22, 2025

Fix to some shared mini cluster ITs #5579

Open

Shared mini test suite minor follow on #5555

Shared mini test suite minor follow on #5555

Uh oh!

Conversation

kevinrr888 commented May 16, 2025

Uh oh!

kevinrr888 May 16, 2025

Choose a reason for hiding this comment

Uh oh!

dlmarion commented May 19, 2025

Uh oh!

kevinrr888 commented May 19, 2025

Uh oh!

ctubbsii May 19, 2025

Choose a reason for hiding this comment

Uh oh!

ctubbsii commented May 19, 2025

Uh oh!

kevinrr888 commented May 19, 2025

Uh oh!

kevinrr888 commented May 20, 2025

Uh oh!

ctubbsii commented May 22, 2025

Uh oh!

ctubbsii May 22, 2025

Choose a reason for hiding this comment

Uh oh!

ctubbsii May 22, 2025

Choose a reason for hiding this comment

Uh oh!

ctubbsii May 22, 2025

Choose a reason for hiding this comment

Uh oh!

ctubbsii May 22, 2025

Choose a reason for hiding this comment

Uh oh!

ctubbsii commented May 22, 2025

Uh oh!

ctubbsii commented May 22, 2025

Uh oh!

ctubbsii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!