Fix testcontainers assume swallowing legit test failures - #4868
Merged
Conversation
…ainers tests
TikaServerExtractionBackendTest, ExtractingRequestHandlerTikaServerTest, and
S3MockContainerRule wrapped container startup in `catch (Throwable t) {
Assume.assumeNoException(...) }`, which silently treated ANY failure -
classpath errors, OOMs, bad configs, not just an absent Docker daemon - as an
environment skip. Replace that with an explicit
`DockerClientFactory.instance().isDockerAvailable()` pre-check via
`Assume.assumeTrue`, matching the pattern already used correctly by
KafkaContainerRule, so a genuine failure now fails the test instead of hiding
as a skip.
Also extract the duplicated Tika container lifecycle (previously hand-rolled
independently in both Tika test classes) into a shared TikaServerContainerRule
`@ClassRule`, mirroring KafkaContainerRule/S3MockContainerRule, so the
Docker-availability check and start/stop logic live in one place.
KafkaContainerRule usages already used lowerCamelCase (kafkaContainer); S3MockContainerRule usages used SCREAMING_SNAKE_CASE (S3_MOCK_RULE) instead. SCREAMING_SNAKE_CASE is conventionally reserved for true compile-time constants, not a static final reference to a stateful, mutable Rule object. Standardize on the KafkaContainerRule convention everywhere: S3_MOCK_RULE -> s3MockContainer, and the new TikaServerContainerRule field -> tikaContainer.
epugh
requested review from
dsmiley and
janhoy
and
a lite review from Copilot
September 3, 2026 12:13
epugh
changed the title
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The refactor consistently applies existing in-repo Testcontainers rule patterns to skip only on true Docker unavailability while allowing legitimate container start failures to surface.
Pull request overview
This pull request updates Solr’s Testcontainers-based integration tests (notably in the extraction and S3 repository modules) to avoid incorrectly skipping tests when real failures occur, while still skipping cleanly when Docker/Testcontainers is genuinely unavailable. It standardizes container lifecycle management via reusable JUnit @ClassRule rules and renames rule fields away from SCREAMING_CASE.
Changes:
- Replace broad
Assume.assumeNoException(..., t)catch-all skipping with an explicit Docker availability check (DockerClientFactory.instance().isDockerAvailable()), allowing real container start failures to fail tests. - Introduce
TikaServerContainerRuleand refactor extraction module Tika Server tests to use it via@ClassRule. - Rename S3 container rule fields from
S3_MOCK_RULEtos3MockContainerand update call sites.
File summaries
| File | Description |
|---|---|
| solr/modules/s3-repository/src/test/org/apache/solr/s3/S3OutputStreamTest.java | Renames the shared S3 mock container @ClassRule field and updates client creation call site. |
| solr/modules/s3-repository/src/test/org/apache/solr/s3/S3MockContainerRule.java | Stops swallowing all container start errors; skips only when Docker/Testcontainers is unavailable. |
| solr/modules/s3-repository/src/test/org/apache/solr/s3/S3InstallShardTest.java | Renames @ClassRule field and updates endpoint substitution. |
| solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java | Renames @ClassRule field and updates endpoint substitution. |
| solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java | Renames @ClassRule field and updates endpoint + client usage. |
| solr/modules/s3-repository/src/test/org/apache/solr/s3/AbstractS3ClientTest.java | Renames @ClassRule field and updates proxy endpoint usage. |
| solr/modules/extraction/src/test/org/apache/solr/handler/extraction/TikaServerExtractionBackendTest.java | Replaces manual container lifecycle and per-test assumes with a shared @ClassRule container rule. |
| solr/modules/extraction/src/test/org/apache/solr/handler/extraction/TikaServerContainerRule.java | Adds a reusable JUnit ExternalResource rule for starting/stopping a Tika Server container with Docker availability checks. |
| solr/modules/extraction/src/test/org/apache/solr/handler/extraction/ExtractingRequestHandlerTikaServerTest.java | Refactors to use TikaServerContainerRule and removes broad exception-to-skip behavior. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
dsmiley
reviewed
Sep 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
I found in testing #4851 that we had real errors get ignored by our test container infrastrcuture in the extraction module.
Solution
Refactor our use of test containers to use Rule everywhere and get away from SCREAMING_CASE.
Tests
existing tests
\