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.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
