[
https://issues.apache.org/jira/browse/TIKA-4327?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18072230#comment-18072230
]
ASF GitHub Bot commented on TIKA-4327:
--------------------------------------
Copilot commented on code in PR #2753:
URL: https://github.com/apache/tika/pull/2753#discussion_r3057102215
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-image-module/src/test/java/org/apache/tika/parser/image/JpegParserTest.java:
##########
@@ -38,10 +41,24 @@
import org.apache.tika.parser.ParseContext;
import org.apache.tika.parser.Parser;
+@Isolated
Review Comment:
`TimeZone.setDefault(...)` mutates JVM-global state; `@Isolated` reduces
risk when JUnit parallel execution is enabled, but it may not fully protect
against concurrent access from other tests running in the same JVM (or
background threads spawned by tests). Consider additionally using JUnit’s
resource locking (e.g., a `@ResourceLock` on a dedicated “timezone” resource or
system properties) to explicitly prevent concurrent execution around this
global mutation and document the intent more clearly.
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-image-module/src/test/java/org/apache/tika/parser/image/JpegParserTest.java:
##########
@@ -25,7 +25,10 @@
import java.util.List;
import java.util.TimeZone;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.parallel.Isolated;
Review Comment:
`TimeZone.setDefault(...)` mutates JVM-global state; `@Isolated` reduces
risk when JUnit parallel execution is enabled, but it may not fully protect
against concurrent access from other tests running in the same JVM (or
background threads spawned by tests). Consider additionally using JUnit’s
resource locking (e.g., a `@ResourceLock` on a dedicated “timezone” resource or
system properties) to explicitly prevent concurrent execution around this
global mutation and document the intent more clearly.
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-image-module/src/test/java/org/apache/tika/parser/image/JpegParserTest.java:
##########
@@ -38,10 +41,24 @@
import org.apache.tika.parser.ParseContext;
import org.apache.tika.parser.Parser;
+@Isolated
public class JpegParserTest extends TikaTest {
static TimeZone CURR_TIME_ZONE = TimeZone.getDefault();
private final Parser parser = new JpegParser();
+ private static TimeZone originalTimeZone;
+
+ @BeforeAll
+ static void init() {
+ // metadata-extractor 2.20.0 started making these parsed dates depend
on the JVM default time zone;
+ // force GMT so the assertions remain deterministic across environments
+ originalTimeZone = TimeZone.getDefault();
+ TimeZone.setDefault(TimeZone.getTimeZone("GMT"));
+ }
+ @AfterAll
+ static void tearDown() {
+ TimeZone.setDefault(originalTimeZone);
+ }
Review Comment:
`CURR_TIME_ZONE` and `originalTimeZone` both capture the pre-test default
timezone but via different mechanisms (class initialization vs. `@BeforeAll`).
This duplication increases the chance of confusion and makes future edits
error-prone. Prefer keeping a single “original timezone” field (ideally
`private static final` if feasible, or `private static` set once) and using it
consistently for restore logic (and any assertions that use the original value).
> General updates for 4.0.0
> -------------------------
>
> Key: TIKA-4327
> URL: https://issues.apache.org/jira/browse/TIKA-4327
> Project: Tika
> Issue Type: Task
> Reporter: Tilman Hausherr
> Priority: Major
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
