Skip to content

Produce a better message when test fails #5676

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
findepi merged 5 commits into from
Oct 24, 2020
Merged

Produce a better message when test fails #5676

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9b60fbb
Split test method
findepi Oct 23, 2020
c63f6ff
Produce a better message when test fails
findepi Oct 23, 2020
7108b41
Avoid DateTimeZone.toTimeZone
findepi Oct 23, 2020
f5116b7
Simplify conversion
findepi Oct 23, 2020
094eaa4
Avoid TimeZone.getTimeZone
findepi Oct 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions presto-hive/src/main/java/io/prestosql/plugin/hive/HiveConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,7 @@ public HiveConfig setWriteValidationThreads(int writeValidationThreads)

public DateTimeZone getRcfileDateTimeZone()
{
return DateTimeZone.forTimeZone(TimeZone.getTimeZone(rcfileTimeZone));
return DateTimeZone.forID(rcfileTimeZone);
}

@NotNull
Expand Down Expand Up @@ -582,7 +582,7 @@ public HiveConfig setTextMaxLineLength(DataSize textMaxLineLength)

public DateTimeZone getOrcLegacyDateTimeZone()
{
return DateTimeZone.forTimeZone(TimeZone.getTimeZone(orcLegacyTimeZone));
return DateTimeZone.forID(orcLegacyTimeZone);
}

@NotNull
Expand All @@ -601,7 +601,7 @@ public HiveConfig setOrcLegacyTimeZone(String orcLegacyTimeZone)

public DateTimeZone getParquetDateTimeZone()
{
return DateTimeZone.forTimeZone(TimeZone.getTimeZone(parquetTimeZone));
return DateTimeZone.forID(parquetTimeZone);
}

@NotNull
Expand Down
2 changes: 1 addition & 1 deletion presto-jdbc/src/main/java/io/prestosql/jdbc/AbstractPrestoResultSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ private Date getDate(int columnIndex, DateTimeZone localTimeZone)
// are not compatible with java.sql.Date.
LocalDate localDate = DATE_FORMATTER.parseLocalDate(String.valueOf(value));
Calendar calendar = new GregorianCalendar(localDate.getYear(), localDate.getMonthOfYear() - 1, localDate.getDayOfMonth());
calendar.setTimeZone(TimeZone.getTimeZone(localTimeZone.getID()));
calendar.setTimeZone(TimeZone.getTimeZone(ZoneId.of(localTimeZone.getID())));

return new Date(calendar.getTimeInMillis());
}
Expand Down
2 changes: 1 addition & 1 deletion presto-jdbc/src/test/java/io/prestosql/jdbc/TestJdbcPreparedStatement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,7 +617,7 @@ public void testConvertTimestamp()
assertParameter(sqlTimestamp, Types.TIMESTAMP, (ps, i) -> ps.setTimestamp(i, sqlTimestamp));
assertParameter(sqlTimestamp, Types.TIMESTAMP, (ps, i) -> ps.setTimestamp(i, sqlTimestamp, null));
assertParameter(sqlTimestamp, Types.TIMESTAMP, (ps, i) -> ps.setTimestamp(i, sqlTimestamp, Calendar.getInstance()));
assertParameter(sameInstantInWarsawZone, Types.TIMESTAMP, (ps, i) -> ps.setTimestamp(i, sqlTimestamp, Calendar.getInstance(TimeZone.getTimeZone("Europe/Warsaw"))));
assertParameter(sameInstantInWarsawZone, Types.TIMESTAMP, (ps, i) -> ps.setTimestamp(i, sqlTimestamp, Calendar.getInstance(TimeZone.getTimeZone(ZoneId.of("Europe/Warsaw")))));
assertParameter(sqlTimestamp, Types.TIMESTAMP, (ps, i) -> ps.setObject(i, sqlTimestamp));
assertParameter(new Timestamp(sqlDate.getTime()), Types.TIMESTAMP, (ps, i) -> ps.setObject(i, sqlDate, Types.TIMESTAMP));
assertParameter(new Timestamp(sqlTime.getTime()), Types.TIMESTAMP, (ps, i) -> ps.setObject(i, sqlTime, Types.TIMESTAMP));
Expand Down
4 changes: 2 additions & 2 deletions presto-jdbc/src/test/java/io/prestosql/jdbc/TestJdbcResultSetTimezone.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ public void testTimestampRoundTrip(String sessionTimezoneId)
assertParameter(sqlTimestamp, sessionTimezoneId, (ps, i) -> ps.setTimestamp(i, sqlTimestamp));
assertParameter(sqlTimestamp, sessionTimezoneId, (ps, i) -> ps.setTimestamp(i, sqlTimestamp, null));
assertParameter(sqlTimestamp, sessionTimezoneId, (ps, i) -> ps.setTimestamp(i, sqlTimestamp, Calendar.getInstance()));
assertParameter(sameInstantInWarsawZone, sessionTimezoneId, (ps, i) -> ps.setTimestamp(i, sqlTimestamp, Calendar.getInstance(TimeZone.getTimeZone("Europe/Warsaw"))));
assertParameter(sameInstantInWarsawZone, sessionTimezoneId, (ps, i) -> ps.setTimestamp(i, sqlTimestamp, Calendar.getInstance(TimeZone.getTimeZone(ZoneId.of("Europe/Warsaw")))));
assertParameter(sqlTimestamp, sessionTimezoneId, (ps, i) -> ps.setObject(i, sqlTimestamp));
assertParameter(new Timestamp(sqlDate.getTime()), sessionTimezoneId, (ps, i) -> ps.setObject(i, sqlDate, Types.TIMESTAMP));
assertParameter(new Timestamp(sqlTime.getTime()), sessionTimezoneId, (ps, i) -> ps.setObject(i, sqlTime, Types.TIMESTAMP));
Expand Down Expand Up @@ -272,7 +272,7 @@ private ResultSet prestoQuery(String expression, String timezoneId)

private Calendar getCalendar()
{
return Calendar.getInstance(TimeZone.getTimeZone(OTHER_TIMEZONE));
return Calendar.getInstance(TimeZone.getTimeZone(ZoneId.of(OTHER_TIMEZONE)));
}

private ZoneId getZoneId()
Expand Down
3 changes: 2 additions & 1 deletion presto-jdbc/src/test/java/io/prestosql/jdbc/TestPrestoDriver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import java.sql.Time;
import java.sql.Timestamp;
import java.sql.Types;
import java.time.ZoneId;
import java.util.ArrayList;
import java.util.GregorianCalendar;
import java.util.List;
Expand Down Expand Up @@ -79,7 +80,7 @@
public class TestPrestoDriver
{
private static final DateTimeZone ASIA_ORAL_ZONE = DateTimeZone.forID("Asia/Oral");
private static final GregorianCalendar ASIA_ORAL_CALENDAR = new GregorianCalendar(ASIA_ORAL_ZONE.toTimeZone());
private static final GregorianCalendar ASIA_ORAL_CALENDAR = new GregorianCalendar(TimeZone.getTimeZone(ZoneId.of(ASIA_ORAL_ZONE.getID())));
private static final String TEST_CATALOG = "test_catalog";

private TestingPrestoServer server;
Expand Down
3 changes: 1 addition & 2 deletions .../src/main/java/io/prestosql/plugin/kafka/encoder/json/format/CustomDateTimeFormatter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@

import java.util.Locale;
import java.util.Optional;
import java.util.TimeZone;

import static io.prestosql.plugin.kafka.encoder.json.format.util.TimeConversions.PICOSECONDS_PER_SECOND;
import static io.prestosql.plugin.kafka.encoder.json.format.util.TimeConversions.getMillisOfDay;
Expand Down Expand Up @@ -98,7 +97,7 @@ public String formatTimestamp(SqlTimestamp value)
@Override
public String formatTimestampWithZone(SqlTimestampWithTimeZone value)
{
DateTimeZone dateTimeZone = DateTimeZone.forTimeZone(TimeZone.getTimeZone(value.getTimeZoneKey().getZoneId()));
DateTimeZone dateTimeZone = DateTimeZone.forID(value.getTimeZoneKey().getId());
return formatter.withZone(dateTimeZone).print(new DateTime(value.getEpochMillis(), dateTimeZone));
}
}
3 changes: 1 addition & 2 deletions ...src/main/java/io/prestosql/plugin/kafka/encoder/json/format/RFC2822DateTimeFormatter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import org.joda.time.format.DateTimeFormatter;

import java.util.Locale;
import java.util.TimeZone;

import static io.prestosql.spi.type.TimestampType.TIMESTAMP_MILLIS;
import static io.prestosql.spi.type.TimestampWithTimeZoneType.TIMESTAMP_TZ_MILLIS;
Expand Down Expand Up @@ -51,7 +50,7 @@ public String formatTimestamp(SqlTimestamp value)
@Override
public String formatTimestampWithZone(SqlTimestampWithTimeZone value)
{
DateTimeZone dateTimeZone = DateTimeZone.forTimeZone(TimeZone.getTimeZone(value.getTimeZoneKey().getZoneId()));
DateTimeZone dateTimeZone = DateTimeZone.forID(value.getTimeZoneKey().getId());
return RFC_FORMATTER.withZone(dateTimeZone).print(new DateTime(value.getEpochMillis(), dateTimeZone));
}
}
2 changes: 1 addition & 1 deletion presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisSessionProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ public static long getTimestampAsMillis(String timestampValue, ConnectorSession
SimpleDateFormat format = new SimpleDateFormat(PRESTO_TIMESTAMP_FORMAT);

if (!session.getTimeZoneKey().getId().equals(TimeZone.getDefault().getID())) {
TimeZone sessionTimeZone = TimeZone.getTimeZone(session.getTimeZoneKey().getId());
TimeZone sessionTimeZone = TimeZone.getTimeZone(session.getTimeZoneKey().getZoneId());
format.setTimeZone(sessionTimeZone);
}

Expand Down
2 changes: 1 addition & 1 deletion presto-main/src/test/java/io/prestosql/operator/scalar/TestDateTimeFunctions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1236,6 +1236,6 @@ private static long millisUtc(OffsetTime offsetTime)

private static SqlTimestampWithTimeZone toTimestampWithTimeZone(DateTime dateTime)
{
return SqlTimestampWithTimeZone.newInstance(3, dateTime.getMillis(), 0, getTimeZoneKey(dateTime.getZone().toTimeZone().getID()));
return SqlTimestampWithTimeZone.newInstance(3, dateTime.getMillis(), 0, getTimeZoneKey(dateTime.getZone().getID()));
}
}
3 changes: 2 additions & 1 deletion presto-main/src/test/java/io/prestosql/type/TestArrayOperators.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import java.text.DateFormat;
import java.text.ParseException;
import java.text.SimpleDateFormat;
import java.time.ZoneId;
import java.util.Collections;
import java.util.List;
import java.util.TimeZone;
Expand Down Expand Up @@ -1868,7 +1869,7 @@ private static SqlDate sqlDate(String dateString)
throws ParseException
{
DateFormat dateFormat = new SimpleDateFormat("yyyy-MM-dd");
dateFormat.setTimeZone(TimeZone.getTimeZone("UTC"));
dateFormat.setTimeZone(TimeZone.getTimeZone(ZoneId.of("UTC")));
return new SqlDate(toIntExact(TimeUnit.MILLISECONDS.toDays(dateFormat.parse(dateString).getTime())));
}
}
2 changes: 1 addition & 1 deletion presto-main/src/test/java/io/prestosql/type/TestTimestamp.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ public void testCastToTimestampWithTimeZone()
{
assertFunction("cast(TIMESTAMP '2001-1-22 03:04:05.321' as timestamp with time zone)",
TIMESTAMP_TZ_MILLIS,
SqlTimestampWithTimeZone.newInstance(3, new DateTime(2001, 1, 22, 3, 4, 5, 321, DATE_TIME_ZONE).getMillis(), 0, TimeZoneKey.getTimeZoneKey(DATE_TIME_ZONE.toTimeZone().getID())));
SqlTimestampWithTimeZone.newInstance(3, new DateTime(2001, 1, 22, 3, 4, 5, 321, DATE_TIME_ZONE).getMillis(), 0, TimeZoneKey.getTimeZoneKey(DATE_TIME_ZONE.getID())));
functionAssertions.assertFunctionString("cast(TIMESTAMP '2001-1-22 03:04:05.321' as timestamp with time zone)",
TIMESTAMP_TZ_MILLIS,
"2001-01-22 03:04:05.321 " + DATE_TIME_ZONE.getID());
Expand Down
28 changes: 24 additions & 4 deletions presto-main/src/test/java/io/prestosql/util/TestTimeZoneUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,20 @@
import java.util.TreeSet;

import static io.prestosql.spi.type.DateTimeEncoding.packDateTimeWithZone;
import static io.prestosql.spi.type.DateTimeEncoding.unpackMillisUtc;
import static io.prestosql.spi.type.DateTimeEncoding.unpackZoneKey;
import static io.prestosql.spi.type.TimeZoneKey.isUtcZoneId;
import static io.prestosql.util.DateTimeZoneIndex.getDateTimeZone;
import static io.prestosql.util.DateTimeZoneIndex.packDateTimeWithZone;
import static io.prestosql.util.DateTimeZoneIndex.unpackDateTimeZone;
import static java.lang.String.format;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.fail;

public class TestTimeZoneUtils
{
@Test
public void test()
public void testNamedZones()
{
TreeSet<String> jdkZones = new TreeSet<>(ZoneId.getAvailableZoneIds());
for (String zoneId : jdkZones) {
Expand All @@ -45,7 +49,11 @@ public void test()
assertDateTimeZoneEquals(zoneId, indexedZone);
assertTimeZone(zoneId, dateTimeZone);
}
}

@Test
public void testOffsets()
{
for (int offsetHours = -13; offsetHours < 14; offsetHours++) {
for (int offsetMinutes = 0; offsetMinutes < 60; offsetMinutes++) {
DateTimeZone dateTimeZone = DateTimeZone.forOffsetHoursMinutes(offsetHours, offsetMinutes);
Expand All @@ -56,9 +64,21 @@ public void test()

public static void assertTimeZone(String zoneId, DateTimeZone dateTimeZone)
{
long dateTimeWithTimeZone = packDateTimeWithZone(new DateTime(42, dateTimeZone));
assertEquals(packDateTimeWithZone(42L, dateTimeZone.toTimeZone().getID()), dateTimeWithTimeZone);
DateTimeZone unpackedZone = unpackDateTimeZone(dateTimeWithTimeZone);
long packWithDateTime = packDateTimeWithZone(new DateTime(42, dateTimeZone));
long packWithZoneId = packDateTimeWithZone(42L, ZoneId.of(dateTimeZone.getID()).getId());
if (packWithDateTime != packWithZoneId) {
fail(format(
"packWithDateTime and packWithZoneId differ for zone [%s] / [%s]: %s [%s %s] and %s [%s %s]",
zoneId,
dateTimeZone,
packWithDateTime,
unpackMillisUtc(packWithDateTime),
unpackZoneKey(packWithDateTime),
packWithZoneId,
unpackMillisUtc(packWithZoneId),
unpackZoneKey(packWithZoneId)));
}
DateTimeZone unpackedZone = unpackDateTimeZone(packWithDateTime);
assertDateTimeZoneEquals(zoneId, unpackedZone);
}

Expand Down
2 changes: 1 addition & 1 deletion presto-oracle/src/main/java/io/prestosql/plugin/oracle/OracleClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ public static LongWriteFunction oracleDateWriteFunction()
public static LongWriteFunction oracleTimestampWriteFunction()
{
return (statement, index, utcMillis) -> {
statement.setObject(index, new oracle.sql.TIMESTAMP(new Timestamp(epochMicrosToMillisWithRounding(utcMillis)), Calendar.getInstance(TimeZone.getTimeZone("UTC"))));
statement.setObject(index, new oracle.sql.TIMESTAMP(new Timestamp(epochMicrosToMillisWithRounding(utcMillis)), Calendar.getInstance(TimeZone.getTimeZone(ZoneId.of("UTC")))));
};
}

Expand Down
2 changes: 1 addition & 1 deletion presto-orc/src/main/java/io/prestosql/orc/OrcRecordReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ public OrcRecordReader(

stripeReader = new StripeReader(
orcDataSource,
legacyFileTimeZone.toTimeZone().toZoneId(),
ZoneId.of(legacyFileTimeZone.getID()),
decompressor,
orcTypes,
ImmutableSet.copyOf(readColumns),
Expand Down
3 changes: 1 addition & 2 deletions presto-orc/src/main/java/io/prestosql/orc/metadata/OrcMetadataReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
import java.util.Map;
import java.util.Optional;
import java.util.OptionalInt;
import java.util.TimeZone;

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Strings.emptyToNull;
Expand Down Expand Up @@ -172,7 +171,7 @@ public StripeFooter readStripeFooter(ColumnMetadata<OrcType> types, InputStream
toStream(stripeFooter.getStreamsList()),
toColumnEncoding(stripeFooter.getColumnsList()),
Optional.ofNullable(emptyToNull(stripeFooter.getWriterTimezone()))
.map(zone -> TimeZone.getTimeZone(zone).toZoneId())
.map(ZoneId::of)
.orElse(legacyFileTimeZone));
}

Expand Down
12 changes: 12 additions & 0 deletions src/modernizer/violations.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,18 @@
<comment>Prefer new Configuration(false)</comment>
</violation>

<violation>
<name>java/util/TimeZone.getTimeZone:(Ljava/lang/String;)Ljava/util/TimeZone;</name>
<version>1.8</version>
<comment>Avoid TimeZone.getTimeZone as it returns GMT for a zone not supported by the JVM. Use TimeZone.getTimeZone(ZoneId.of(..)) instead, or TimeZone.getTimeZone(..., false).</comment>
</violation>

<violation>
<name>org/joda/time/DateTimeZone.toTimeZone:()Ljava/util/TimeZone;</name>
<version>1.8</version>
<comment>Avoid DateTimeZone.toTimeZone as it returns GMT for a zone not supported by the JVM. Use TimeZone.getTimeZone(ZoneId.of(dtz.getId())) instead.</comment>
Copy link
Copy Markdown
Member

@martint martint Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as it returns GMT for a zone not supported by the JVM

wow!

Copy link
Copy Markdown
Member Author

@findepi findepi Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will likewise block use of TimeZone.forTimeZone(String).

</violation>

<violation>
<name>com/esri/core/geometry/ogc/OGCGeometry.equals:(Lcom/esri/core/geometry/ogc/OGCGeometry;)Z</name>
<version>1.6</version>
Expand Down