[
https://issues.apache.org/jira/browse/TIKA-4327?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18072223#comment-18072223
]
ASF GitHub Bot commented on TIKA-4327:
--------------------------------------
Copilot commented on code in PR #2753:
URL: https://github.com/apache/tika/pull/2753#discussion_r3056987349
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-image-module/src/test/java/org/apache/tika/parser/image/JpegParserTest.java:
##########
@@ -42,6 +44,17 @@ public class JpegParserTest extends TikaTest {
static TimeZone CURR_TIME_ZONE = TimeZone.getDefault();
private final Parser parser = new JpegParser();
+ static private TimeZone timezone;
+
+ @BeforeAll
+ static void init() {
+ timezone = TimeZone.getDefault();
+ TimeZone.setDefault(TimeZone.getTimeZone("GMT")); // need to set TZ
since extractor 2.20.0, PR# 651
+ }
+ @AfterAll
+ static void tearDown() {
+ TimeZone.setDefault(timezone);
+ }
Review Comment:
Changing the JVM default timezone is a global mutation and can make the
build flaky if tests are executed in parallel across classes/modules (other
tests may observe GMT mid-run). If the project runs JUnit 5 tests in parallel,
consider isolating this test class (e.g., JUnit Jupiter `@Isolated` or a
`@ResourceLock` guarding timezone/global state) or otherwise ensuring this
module/class is excluded from parallel execution.
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-image-module/src/test/java/org/apache/tika/parser/image/JpegParserTest.java:
##########
@@ -42,6 +44,17 @@ public class JpegParserTest extends TikaTest {
static TimeZone CURR_TIME_ZONE = TimeZone.getDefault();
private final Parser parser = new JpegParser();
+ static private TimeZone timezone;
+
+ @BeforeAll
+ static void init() {
+ timezone = TimeZone.getDefault();
+ TimeZone.setDefault(TimeZone.getTimeZone("GMT")); // need to set TZ
since extractor 2.20.0, PR# 651
Review Comment:
The inline comment isn’t very actionable (\"PR# 651\" is ambiguous without a
link/repo context). Consider referencing the concrete upstream change (link to
the metadata-extractor PR/release note) and/or the local tracking issue (e.g.,
TIKA-4327) so future maintainers can understand why GMT is required here.
```suggestion
TimeZone.setDefault(TimeZone.getTimeZone("GMT")); //
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
```
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-image-module/src/test/java/org/apache/tika/parser/image/JpegParserTest.java:
##########
@@ -42,6 +44,17 @@ public class JpegParserTest extends TikaTest {
static TimeZone CURR_TIME_ZONE = TimeZone.getDefault();
private final Parser parser = new JpegParser();
+ static private TimeZone timezone;
+
+ @BeforeAll
+ static void init() {
+ timezone = TimeZone.getDefault();
+ TimeZone.setDefault(TimeZone.getTimeZone("GMT")); // need to set TZ
since extractor 2.20.0, PR# 651
+ }
+ @AfterAll
+ static void tearDown() {
+ TimeZone.setDefault(timezone);
+ }
Review Comment:
Timezone is captured twice (`CURR_TIME_ZONE` at class initialization and
`timezone` in `@BeforeAll`), which is redundant and can be confusing about
which value is authoritative. Consider consolidating to a single field captured
in `@BeforeAll` (or reuse `CURR_TIME_ZONE` and remove `timezone`) and keep the
modifier order conventional (e.g., `private static TimeZone timezone;`) to
align with typical Java style.
> 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)
