ppkarwasz commented on code in PR #3888:
URL: https://github.com/apache/logging-log4j2/pull/3888#discussion_r2285248663
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/pattern/NamedInstantPatternTest.java:
##########
@@ -40,6 +41,19 @@ void compatibilityOfLegacyPattern(NamedInstantPattern
namedPattern) {
Instant javaTimeInstant = Instant.now();
MutableInstant instant = new MutableInstant();
instant.initFromEpochSecond(javaTimeInstant.getEpochSecond(),
javaTimeInstant.getNano());
-
assertThat(legacyFormatter.format(instant)).isEqualTo(formatter.format(instant));
+ String legacy = legacyFormatter.format(instant);
+ String modern = formatter.format(instant);
+ if (namedPattern == NamedInstantPattern.ISO8601_OFFSET_DATE_TIME_HH) {
+ java.time.ZoneOffset offset =
+
java.time.ZoneId.systemDefault().getRules().getOffset(java.time.Instant.now());
+ Assumptions.assumeTrue(
+ offset.getTotalSeconds() % 3600 == 0,
+ () -> String.format(
+ "Skipping test: ISO8601_OFFSET_DATE_TIME_HH
requires a whole-hour offset, but system offset is %s",
+ offset));
+ assertThat(legacy).isEqualTo(modern);
+ } else {
Review Comment:
The assumption check should be the very first thing in the test. That way,
if it fails, *none* of the setup or assertions will run. Since
`Assumptions.assumeTrue(...)` throws an exception when the condition is not
met, you don’t need to wrap later code in an `if` block — the test will be
skipped automatically. This also makes the test flow easier to follow.
_Nitpick_: personally I prefer AssertJ over pure JUnit5. It also has an
`Assumptions` class.
--
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: notifications-unsubscr...@logging.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
