Skip to content
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

Jump to bottom

Enhancing Test Case Granularity for testGetLocalDateTimeTypes #2360

Closed
wants to merge 3 commits into from
Closed

Enhancing Test Case Granularity for testGetLocalDateTimeTypes #2360

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a40e60d
Split testGetLocalDateTimeTypes into two test case to increase the te…
Codegass Mar 21, 2024
3f0ad83
Put the extracted Arrangement functions back to the tests.
Codegass Apr 15, 2024
93e95d8
add back the comments
Codegass Apr 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 21 additions & 12 deletions src/test/java/com/microsoft/sqlserver/jdbc/datatypes/DataTypesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1874,38 +1874,36 @@ public void testNullValuesWithGetObject() throws Exception {
}

@Test
public void testGetLocalDateTimeTypes() throws Exception {
// test value needs to be in a time zone other than local
OffsetDateTime value = OffsetDateTime.now().truncatedTo(ChronoUnit.MILLIS); // Linux has more precision than SQL Server
int offsetSeconds = value.getOffset().getTotalSeconds();
offsetSeconds += offsetSeconds < 0 ? 3600 : -3600;
value = value.withOffsetSameLocal(ZoneOffset.ofTotalSeconds(offsetSeconds));
LocalDateTime valueWithOffsetConversion = value.atZoneSameInstant(ZoneId.systemDefault()).toLocalDateTime();
public void testGetLocalDateTimeTypesWithDefaultOffsetDateTimeConversion() throws Exception {
OffsetDateTime value = prepareOffsetDateTime();
LocalDateTime valueWithOffsetConversion = convertOffsetDateTimeToLocalDateTime(value);

try (SQLServerConnection conn = PrepUtil.getConnection(connectionString)) {
try (PreparedStatement stmt = conn.prepareStatement("SELECT ?")) {
stmt.setObject(1, value);

ResultSet rs = stmt.executeQuery();
rs.next();

// default behavior is to apply the time zone offset when converting DATETIMEOFFSET to local java.time types
assertEquals(value, rs.getObject(1, OffsetDateTime.class));
assertEquals(valueWithOffsetConversion, rs.getObject(1, LocalDateTime.class));
assertEquals(valueWithOffsetConversion.toLocalDate(), rs.getObject(1, LocalDate.class));
assertEquals(valueWithOffsetConversion.toLocalTime(), rs.getObject(1, LocalTime.class));
}
}
}

@Test
public void testGetLocalDateTimeTypesWithIgnoreOffsetDateTimeConversion() throws Exception {
OffsetDateTime value = prepareOffsetDateTime();

// change the behavior to be compatible with java.time conversion methods
try (SQLServerConnection conn = PrepUtil.getConnection(connectionString)) {
conn.setIgnoreOffsetOnDateTimeOffsetConversion(true);

try (PreparedStatement stmt = conn.prepareStatement("SELECT ?")) {
stmt.setObject(1, value);

ResultSet rs = stmt.executeQuery();
rs.next();

// now the offset should be ignored instead of converting to local time zone
assertEquals(value, rs.getObject(1, OffsetDateTime.class));
assertEquals(value.toLocalDateTime(), rs.getObject(1, LocalDateTime.class));
assertEquals(value.toLocalDate(), rs.getObject(1, LocalDate.class));
Expand Down Expand Up @@ -1946,6 +1944,17 @@ public void testDateTimeOffsetValueOfOffsetDateTime() throws Exception {
assertEquals(expected, DateTimeOffset.valueOf(roundDown).getOffsetDateTime());
}

private OffsetDateTime prepareOffsetDateTime() {
OffsetDateTime value = OffsetDateTime.now().truncatedTo(ChronoUnit.MILLIS);
int offsetSeconds = value.getOffset().getTotalSeconds();
offsetSeconds += offsetSeconds < 0 ? 3600 : -3600;
return value.withOffsetSameLocal(ZoneOffset.ofTotalSeconds(offsetSeconds));
}

private LocalDateTime convertOffsetDateTimeToLocalDateTime(OffsetDateTime value) {
return value.atZoneSameInstant(ZoneId.systemDefault()).toLocalDateTime();
}

Copy link
Member

Choose a reason for hiding this comment

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

I'm not opposed to splitting the tests but refactoring into these methods seem unnecessary since they are only called once, why not just include them in the tests themselves?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback! The decision to refactor was driven by enhancing test readability and making future maintenance easier. This approach will improve the clarity of our test cases and reduces debugging rerun time when debugging for specific scenario. When tests are more granular and focused, identifying and addressing issues becomes faster, leading to more efficient debugging cycles.

Even though the methods are called only once now, this structure allows us to easily expand or modify tests with minimal changes. The reduction in debugging rerun time and the enhancement in clarity are pivotal, as they ensure that our test suite remains robust, facilitating quicker modifications.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it makes it any easier to read. If this changes in the future it can be changed then for now there is no need to do unnecessary refactoring and extra method calls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for suggestion! Your perspective on the overall development picture is very helpful. I agree that maintainability should be balanced with immediate clarity. Let's keep the tests straightforward as you've advised.

static LocalDateTime getUnstorableValue() throws Exception {
ZoneId systemTimezone = ZoneId.systemDefault();
Instant now = Instant.now();
Expand Down