You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tika.apache.org by "THausherr (via GitHub)" <gi...@apache.org> on 2026/04/05 17:10:27 UTC
[PR] [TIKA-4704] Implement tearDown method in UnpackerResourceTest [tika]
THausherr opened a new pull request, #2735:
URL: https://github.com/apache/tika/pull/2735
Add tearDown method to clean up temporary directory after tests.
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for your contribution to [Apache Tika](https://tika.apache.org/)! Your help is appreciated!
Before opening the pull request, please verify that
* there is an open issue on the [Tika issue tracker](https://issues.apache.org/jira/projects/TIKA) which describes the problem or the improvement. We cannot accept pull requests without an issue because the change wouldn't be listed in the release notes.
* the issue ID (`TIKA-XXXX`)
- is referenced in the title of the pull request
- and placed in front of your commit messages surrounded by square brackets (`[TIKA-XXXX] Issue or pull request title`)
* commits are squashed into a single one (or few commits for larger changes)
* Tika is successfully built and unit tests pass by running `./mvnw clean test`
* there should be no conflicts when merging the pull request branch into the *recent* `main` branch. If there are conflicts, please try to rebase the pull request branch on top of a freshly pulled `main` branch
* if you add new module that downstream users will depend upon add it to relevant group in `tika-bom/pom.xml`.
We will be able to faster integrate your pull request if these conditions are met. If you have any questions how to fix your problem or about using Tika in general, please sign up for the [Tika mailing list](http://tika.apache.org/mail-lists.html). Thanks!
--
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: dev-unsubscribe@tika.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TIKA-4704] Implement tearDown method to remove temp directories [tika]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2735:
URL: https://github.com/apache/tika/pull/2735#discussion_r3037196150
##########
tika-server/tika-server-standard/src/test/java/org/apache/tika/server/standard/UnpackerResourceWithConfigTest.java:
##########
@@ -260,4 +262,13 @@ public void testPDFPerPageRenderGray() throws Exception {
assertTrue(averageColor.getBlue() < 100 && averageColor.getBlue() > 90);
}
}
+
+ @AfterAll
+ @Override
+ public void tearDown() throws Exception {
+ super.tearDown();
+ if (unpackTempDir != null && Files.exists(unpackTempDir)) {
+ FileUtils.deleteDirectory(unpackTempDir.toFile());
Review Comment:
Minor: consider using try/finally around `super.tearDown()` so `unpackTempDir` deletion is attempted even if superclass teardown throws.
```suggestion
try {
super.tearDown();
} finally {
if (unpackTempDir != null && Files.exists(unpackTempDir)) {
FileUtils.deleteDirectory(unpackTempDir.toFile());
}
```
--
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: dev-unsubscribe@tika.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TIKA-4704] Implement tearDown method to remove temp directories [tika]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2735:
URL: https://github.com/apache/tika/pull/2735#discussion_r3037111465
##########
tika-server/tika-server-standard/src/test/java/org/apache/tika/server/standard/UnpackerResourceTest.java:
##########
@@ -602,4 +603,9 @@ public void testShallowExtraction() throws Exception {
assertEquals(200, response.getStatus());
// Just verify it succeeds - actual depth limiting behavior depends on document structure
}
+
+ @AfterAll
+ public void tearDown() throws IOException {
+ Files.delete(unpackTempDir);
Review Comment:
`unpackTempDir` is created via `Files.createTempDirectory(...)`, so it will typically contain files after the unpack tests run. `Files.delete(unpackTempDir)` will fail with `DirectoryNotEmptyException` in that case. Use a recursive directory delete (e.g., `FileUtils.deleteDirectory(unpackTempDir.toFile())` or a `Files.walk(...).sorted(reverseOrder())` delete) and prefer `deleteIfExists`/null guarding to avoid masking test failures with cleanup exceptions.
```suggestion
if (unpackTempDir != null) {
try (java.util.stream.Stream<Path> paths = Files.walk(unpackTempDir)) {
paths.sorted(java.util.Comparator.reverseOrder())
.forEach(path -> {
try {
Files.deleteIfExists(path);
} catch (IOException e) {
throw new RuntimeException(e);
}
});
} catch (RuntimeException e) {
if (e.getCause() instanceof IOException) {
throw (IOException) e.getCause();
}
throw e;
}
}
```
##########
tika-server/tika-server-standard/src/test/java/org/apache/tika/server/standard/UnpackerResourceTest.java:
##########
@@ -602,4 +603,9 @@ public void testShallowExtraction() throws Exception {
assertEquals(200, response.getStatus());
// Just verify it succeeds - actual depth limiting behavior depends on document structure
}
+
+ @AfterAll
+ public void tearDown() throws IOException {
Review Comment:
This `tearDown()` method overrides `CXFTestBase#tearDown()` (which is also annotated with `@AfterAll`) but doesn’t call `super.tearDown()`. That prevents the base cleanup from running (server stop/destroy, PipesParser close, config/temp cleanup), which can leave resources open and make subsequent tests flaky. Consider either renaming this method (e.g., `cleanUpUnpackTempDir`) so it doesn’t override, or add `@Override`, widen the signature to `throws Exception`, and call `super.tearDown()` before deleting the temp directory.
```suggestion
@Override
@AfterAll
public void tearDown() throws Exception {
super.tearDown();
```
--
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: dev-unsubscribe@tika.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TIKA-4704] Implement tearDown method to remove temp directories [tika]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2735:
URL: https://github.com/apache/tika/pull/2735#discussion_r3037161116
##########
tika-server/tika-server-standard/src/test/java/org/apache/tika/server/standard/UnpackerResourceWithConfigTest.java:
##########
@@ -260,4 +262,12 @@ public void testPDFPerPageRenderGray() throws Exception {
assertTrue(averageColor.getBlue() < 100 && averageColor.getBlue() > 90);
}
}
+
+ @AfterAll
+ public void tearDown() throws Exception {
+ super.tearDown();
+ if (unpackTempDir != null && Files.exists(unpackTempDir)) {
+ FileUtils.deleteQuietly(unpackTempDir.toFile());
Review Comment:
`FileUtils.deleteQuietly(...)` will silently ignore failures, which can leave the temp unpack directory behind with no signal (potentially defeating the purpose of TIKA-4704). Consider using `FileUtils.deleteDirectory(...)` (or catching/logging any IOException) so cleanup failures are visible and consistent with other tests in this module (e.g., `TikaPipesTest`). Also, adding `@Override` here would make it explicit that this method overrides `CXFTestBase#tearDown`.
```suggestion
@Override
public void tearDown() throws Exception {
super.tearDown();
if (unpackTempDir != null && Files.exists(unpackTempDir)) {
FileUtils.deleteDirectory(unpackTempDir.toFile());
```
--
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: dev-unsubscribe@tika.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TIKA-4704] Implement tearDown method to remove temp directories [tika]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2735:
URL: https://github.com/apache/tika/pull/2735#discussion_r3037175283
##########
tika-server/tika-server-standard/src/test/java/org/apache/tika/server/standard/UnpackerResourceTest.java:
##########
@@ -602,4 +604,13 @@ public void testShallowExtraction() throws Exception {
assertEquals(200, response.getStatus());
// Just verify it succeeds - actual depth limiting behavior depends on document structure
}
+
Review Comment:
There’s trailing whitespace on the blank line before the new tearDown() method. This can cause style/formatting checks to fail; please remove the spaces so the line is truly empty.
```suggestion
```
##########
tika-server/tika-server-standard/src/test/java/org/apache/tika/server/standard/UnpackerResourceTest.java:
##########
@@ -602,4 +604,13 @@ public void testShallowExtraction() throws Exception {
assertEquals(200, response.getStatus());
// Just verify it succeeds - actual depth limiting behavior depends on document structure
}
+
+ @Override
+ @AfterAll
+ public void tearDown() throws Exception {
+ super.tearDown();
+ if (unpackTempDir != null && Files.exists(unpackTempDir)) {
+ FileUtils.deleteDirectory(unpackTempDir.toFile());
+ }
+ }
Review Comment:
This cleanup logic is duplicated across multiple tests and requires an extra commons-io dependency/import. Consider switching unpackTempDir to JUnit’s `@TempDir` (as done in tika-server-core StackTraceTest) so the temp directory lifecycle is managed automatically and you can drop the manual FileUtils.deleteDirectory() tearDown code.
##########
tika-server/tika-server-standard/src/test/java/org/apache/tika/server/standard/UnpackerResourceWithConfigTest.java:
##########
@@ -260,4 +262,13 @@ public void testPDFPerPageRenderGray() throws Exception {
assertTrue(averageColor.getBlue() < 100 && averageColor.getBlue() > 90);
}
}
+
+ @AfterAll
+ @Override
+ public void tearDown() throws Exception {
+ super.tearDown();
+ if (unpackTempDir != null && Files.exists(unpackTempDir)) {
+ FileUtils.deleteDirectory(unpackTempDir.toFile());
+ }
+ }
Review Comment:
This manual temp-dir cleanup is duplicated with UnpackerResourceTest and pulls in commons-io FileUtils. Consider using JUnit’s `@TempDir` for unpackTempDir (see tika-server-core StackTraceTest) so the temp directory is cleaned up automatically and you can remove the custom tearDown() deletion logic.
--
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: dev-unsubscribe@tika.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TIKA-4704] Implement tearDown method to remove temp directories [tika]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2735:
URL: https://github.com/apache/tika/pull/2735#discussion_r3037167708
##########
tika-server/tika-server-standard/src/test/java/org/apache/tika/server/standard/UnpackerResourceWithConfigTest.java:
##########
@@ -260,4 +262,12 @@ public void testPDFPerPageRenderGray() throws Exception {
assertTrue(averageColor.getBlue() < 100 && averageColor.getBlue() > 90);
}
}
+
+ @AfterAll
+ public void tearDown() throws Exception {
+ super.tearDown();
+ if (unpackTempDir != null && Files.exists(unpackTempDir)) {
+ FileUtils.deleteDirectory(unpackTempDir.toFile());
Review Comment:
Add `@Override` on this `tearDown()` method to ensure the signature is checked against `CXFTestBase.tearDown()` and to avoid accidentally ending up with two `@AfterAll` methods if the signature ever changes.
--
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: dev-unsubscribe@tika.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TIKA-4704] Implement tearDown method to remove temp directories [tika]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2735:
URL: https://github.com/apache/tika/pull/2735#discussion_r3037153117
##########
tika-server/tika-server-standard/src/test/java/org/apache/tika/server/standard/UnpackerResourceTest.java:
##########
@@ -602,4 +604,11 @@ public void testShallowExtraction() throws Exception {
assertEquals(200, response.getStatus());
// Just verify it succeeds - actual depth limiting behavior depends on document structure
}
+
+ @Override
+ @AfterAll
+ public void tearDown() throws Exception {
+ super.tearDown();
+ FileUtils.deleteDirectory(unpackTempDir.toFile());
Review Comment:
`unpackTempDir` is deleted unconditionally here. If `@BeforeAll` fails before `unpackTempDir` is assigned (e.g., config load error), this `@AfterAll` can throw a `NullPointerException` and mask the original failure. Consider guarding with a null/existence check before deleting the directory (similar to the pattern used in `TikaPipesTest`).
```suggestion
if (unpackTempDir != null && Files.exists(unpackTempDir)) {
FileUtils.deleteDirectory(unpackTempDir.toFile());
}
```
##########
tika-server/tika-server-standard/src/test/java/org/apache/tika/server/standard/UnpackerResourceWithConfigTest.java:
##########
@@ -260,4 +262,10 @@ public void testPDFPerPageRenderGray() throws Exception {
assertTrue(averageColor.getBlue() < 100 && averageColor.getBlue() > 90);
}
}
+
+ @AfterAll
+ public void tearDown() throws IOException {
Review Comment:
This method declares `throws IOException`, but it calls `super.tearDown()` which is declared `throws Exception` in `CXFTestBase`. That won’t compile unless you either broaden this method’s throws clause to `Exception` (or `Throwable`-compatible) or catch/handle the checked `Exception` from `super.tearDown()`.
```suggestion
public void tearDown() throws Exception {
```
##########
tika-server/tika-server-standard/src/test/java/org/apache/tika/server/standard/UnpackerResourceWithConfigTest.java:
##########
@@ -260,4 +262,10 @@ public void testPDFPerPageRenderGray() throws Exception {
assertTrue(averageColor.getBlue() < 100 && averageColor.getBlue() > 90);
}
}
+
+ @AfterAll
+ public void tearDown() throws IOException {
+ super.tearDown();
+ FileUtils.deleteDirectory(unpackTempDir.toFile());
Review Comment:
Same as in `UnpackerResourceTest`: deleting `unpackTempDir` unconditionally in `@AfterAll` can throw (NPE / missing dir) if setup fails early, which can hide the real test failure. Add a null/existence guard (and possibly a best-effort delete) around this cleanup.
```suggestion
if (unpackTempDir != null && Files.exists(unpackTempDir)) {
FileUtils.deleteQuietly(unpackTempDir.toFile());
}
```
--
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: dev-unsubscribe@tika.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TIKA-4704] Implement tearDown method to remove temp directories [tika]
Posted by "THausherr (via GitHub)" <gi...@apache.org>.
THausherr merged PR #2735:
URL: https://github.com/apache/tika/pull/2735
--
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: dev-unsubscribe@tika.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org