Skip to content

Shared mini test suite #5536

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
May 15, 2025
Merged

Shared mini test suite #5536

merged 15 commits into from
May 15, 2025

Conversation

dlmarion
Copy link
Contributor

@dlmarion dlmarion commented May 6, 2025

Created SimpleSharedMacTestSuiteIT which starts Mini, runs all of the tests with
the SIMPLE_MINI_CLUSTER_SUITE tag, then shuts down Mini. Annotated most of
the tests that extended SharedMiniClusterBase and called startMiniCluster with
no configuration callback with the SIMPLE_MINI_CLUSTER_SUITE tag. These test
with that did not use a configuration callback where most likely to not do
destructive things with Mini. ManagerApiIT was one exception and I did not put
the tag on this class.

Modified SharedMiniClusterBase so that stopMiniCluster would not stop the
cluster if STOP_DISABLED was set to true, which it is in the new test suite class.
This enables a single test class to be run and behave normally, but also allows
the class to be part of the test suite and not shut down Mini after the tests have
run. Modified stopMiniCluster to instead delete all user tables when STOP_DISABLED
is set to true to clean up tables from the test class. Leaving the tables around
in the instance worked for most tests, but not for tests that counted tables
and expected a certain number (CreateTableIT) or for tests that kicked off a compaction
and waited for it to finish (ComprehensiveIT.testCompaction) as the single
Compactor may have been busy compacting other tables.

Modified the pom to have two failsafe executions, one for the ITs without the
SIMPLE_MINI_CLUSTER_SUITE and one for the test suite.

Closes #5529

dlmarion added 5 commits May 1, 2025 20:41
All ITs that extend SharedMiniClusterBase, but start it without
the callback parameter can likely be run together. This commit
creates a new class, SimpleSharedMacTestSuite, which will set
up the shared MiniAccumuloCluster before running all tests that
have the SIMPLE_MINI_CLUSTER_ONLY tag, and then shut the cluster
down.
@dlmarion dlmarion added this to the 4.0.0 milestone May 6, 2025
@dlmarion dlmarion self-assigned this May 6, 2025
@dlmarion
Copy link
Contributor Author

dlmarion commented May 7, 2025

Testing locally using mvn clean verify with 0d19981 and it runs the new test suite first. Saw two tests fail in the test suite:

[ERROR] Tests run: 13, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 15.85 s <<< FAILURE! -- in org.apache.accumulo.test.fate.user.UserFateStoreFateIT
[ERROR] org.apache.accumulo.test.fate.user.UserFateStoreFateIT.testCreateWithKeyInProgress -- Time elapsed: 0.630 s <<< ERROR!
java.lang.IllegalStateException: Attempted to reserve a tx that does not exist: FATE:USER:1036c868-7b72-3aa2-8e55-a807d1518a24
	at com.google.common.base.Preconditions.checkState(Preconditions.java:513)
	at org.apache.accumulo.core.fate.AbstractFateStore.reserve(AbstractFateStore.java:148)
	at org.apache.accumulo.test.fate.FateStoreIT.testCreateWithKeyInProgress(FateStoreIT.java:396)
	at org.apache.accumulo.test.fate.user.UserFateStoreFateIT.executeTest(UserFateStoreFateIT.java:63)
	at org.apache.accumulo.test.fate.FateTestRunner.executeTest(FateTestRunner.java:33)
	at org.apache.accumulo.test.fate.FateStoreIT.testCreateWithKeyInProgress(FateStoreIT.java:364)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:840)

and

[ERROR] org.apache.accumulo.test.CloneIT.testClonedMarker(Range, Range, Range)[2] -- Time elapsed: 2.014 s <<< ERROR!
java.lang.IllegalStateException: No prev endrow seen.  tableId: 0 endrow: m
	at org.apache.accumulo.core.metadata.schema.TabletMetadata.getPrevEndRow(TabletMetadata.java:488)
	at org.apache.accumulo.core.metadata.schema.TabletMetadata.lambda$new$1(TabletMetadata.java:190)
	at com.google.common.base.Suppliers$NonSerializableMemoizingSupplier.get(Suppliers.java:198)
	at org.apache.accumulo.core.metadata.schema.TabletMetadata.getExtent(TabletMetadata.java:478)
	at org.apache.accumulo.core.metadata.schema.LinkingIterator.goodTransition(LinkingIterator.java:85)
	at org.apache.accumulo.core.metadata.schema.LinkingIterator.next(LinkingIterator.java:174)
	at org.apache.accumulo.core.metadata.schema.LinkingIterator.next(LinkingIterator.java:50)
	at org.apache.accumulo.server.util.MetadataTableUtil.checkClone(MetadataTableUtil.java:297)
	at org.apache.accumulo.test.CloneIT.testClonedMarker(CloneIT.java:386)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:840)

It looks like the ITs that are run as part of the suite are being run again in the second IT run that is meant to exclude them. Need to continue debugging that.

@dlmarion dlmarion marked this pull request as ready for review May 8, 2025 12:43
@dlmarion
Copy link
Contributor Author

dlmarion commented May 8, 2025

@keith-turner @kevinrr888 - the tests in the comment above are consistently failing for me locally with the same error. The change here is that these tests are being run in a MiniAccumuloCluster instance that is much longer lived with other tests happening before the tests that are failing.

@dlmarion
Copy link
Contributor Author

dlmarion commented May 8, 2025

The change in 2efdef7 seems to resolve the tests being run twice. I think I have this where I want it now. I'm going to kick off a full build to see if it sped things up.

Removing the IT suffix should prevent the suite from
getting picked up in the second failsafe execution.
The excludedGroup tag was not working for some reason.
@dlmarion
Copy link
Contributor Author

Full IT build successful. Compared the most recent 4.0 full build:

  1. both had the same number of ITs run (1085).
  2. the IT build for this PR took 7h39m with 95 minutes wasted in timed out ITs
  3. the 4.0 build took 6h36m with 40 minutes wasted in timed out ITs

It's interesting that ScanServerUpgrade11to12TestIT eventually succeeded in the PR build. It's been failing consistently in the 4.0 build for a while now as noted in #5531.

Copy link
Member

@kevinrr888 kevinrr888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So long as Christopher is good with the build changes, I think this is good to go!

I am working on getting this to work for the ITs that set a custom config. Was originally considering just putting that PR up against this branch, but now thinking it will be better suited as a standalone PR after this is merged in (easier for others to review and won't hold up this PR).

@dlmarion dlmarion merged commit 41792a4 into apache:main May 15, 2025
8 checks passed
@dlmarion dlmarion deleted the shared-mini-test-suite branch May 15, 2025 11:47
Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the all-or-nothing approach to the tests in the suite, but if it saves a lot of time in the normal case, it's probably worth it. What happens when you run an individual test in the suite in the IDE? Does it still work correctly? Or do you have to run the suite test and see the output of all the tests in the suite?

Also, I'm not a fan of the fact that it seems to run every time, even when specifying -Dit.test= for a specific test. That can be addressed in a profile, as I mentioned in the below comment.

Comment on lines +1156 to +1165
<execution>
<id>run-integration-test-suite</id>
<goals>
<goal>integration-test</goal>
<goal>verify</goal>
</goals>
<configuration>
<test>org.apache.accumulo.suites.SimpleSharedMacTestSuite</test>
</configuration>
</execution>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this overwrites the user's -Dit.test= selection, and will always run, regardless of which tests the user specifies. I'm not sure a good workaround for that, except maybe put this execution in a profile that is activated when that test variable is not provided, as in <property>!it.test</property>. In that case, it would also be skipped when it.test is used to specify one of the tests in the suite itself, so it's not a perfect solution, but it's probably better to suppress the execution when the user wants to run an explicit test.

Comment on lines +201 to +208
<dependency>
<groupId>org.junit.platform</groupId>
<artifactId>junit-platform-suite-api</artifactId>
</dependency>
<dependency>
<groupId>org.junit.platform</groupId>
<artifactId>junit-platform-suite-engine</artifactId>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these compile time dependencies or runtime dependencies? It seems like maybe the engine is a runtime dependency. It probably should have a runtime scope.

@dlmarion
Copy link
Contributor Author

I don't like the all-or-nothing approach to the tests in the suite, but if it saves a lot of time in the normal case, it's probably worth it. What happens when you run an individual test in the suite in the IDE?

Running a single test that is part of the suite on it's own should still work.

@ctubbsii
Copy link
Member

Running a single test that is part of the suite on it's own should still work.

Okay, good to know!

kevinrr888 added a commit to kevinrr888/accumulo that referenced this pull request May 16, 2025
- 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
kevinrr888 added a commit to kevinrr888/accumulo that referenced this pull request May 16, 2025
- 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 dlmarion mentioned this pull request May 19, 2025
kevinrr888 added a commit that referenced this pull request May 22, 2025
* Minor follow on to #5536

- 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
- Small typo fixes
- Changed junit-platform-suite-engine dependency in test/pom.xml from being compile scope to being test scope
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shared mini cluster across multiple ITs
4 participants