Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request migrates the google-cloud-bigquery-jdbc module's test suite from JUnit 4 to JUnit 5. This involves updating pom.xml to replace JUnit 4 dependencies with JUnit 5 Jupiter dependencies and Mockito's JUnit 5 extension. Test classes were refactored to use JUnit 5 annotations such as @Test, @BeforeEach, @AfterAll, @BeforeAll, @ParameterizedTest, @MethodSource, @RegisterExtension, @ExtendWith, @Nested, @TestInstance, and Assertions for assertions, replacing their JUnit 4 counterparts. Additionally, the TimeZoneRule was adapted to implement JUnit 5 extension interfaces.
| import org.junit.jupiter.api.extension.RegisterExtension; | ||
| import org.junit.jupiter.api.function.Executable; | ||
| import org.junit.jupiter.params.ParameterizedTest; | ||
| import org.junit.jupiter.params.provider.MethodSource; |
There was a problem hiding this comment.
The migration from JUnit 4's @RunWith(Parameterized.class) and @Parameters to JUnit 5's @ParameterizedTest and @MethodSource is correctly implemented. This improves test flexibility and readability.
| @ParameterizedTest(name = "{index}: primitive array of {0}") | ||
| @MethodSource("data") | ||
| public void getArray( | ||
| StandardSQLTypeName currentType, | ||
| Tuple<Field, JsonStringArrayList<?>> schemaAndValue, | ||
| Object[] expected, | ||
| int javaSqlTypeCode) | ||
| throws SQLException { | ||
| Array array = new BigQueryArrowArray(schemaAndValue.x(), schemaAndValue.y()); | ||
| assertThat(array.getArray()).isEqualTo(expected); |
There was a problem hiding this comment.
The test method getArray has been correctly refactored to use @ParameterizedTest and receive its data from the data() method. Instantiating Array array inside the test method ensures proper test isolation for each parameter set.
| @ParameterizedTest(name = "{index}: {0}") | ||
| @MethodSource("data") | ||
| public void stringToBoolean(String label, BigQueryTypeCoercer bigQueryTypeCoercer) { | ||
| assertThat(bigQueryTypeCoercer.coerceTo(Boolean.class, "true")).isTrue(); | ||
| assertThat(bigQueryTypeCoercer.coerceTo(Boolean.class, "false")).isFalse(); |
There was a problem hiding this comment.
The test method stringToBoolean has been correctly refactored to use @ParameterizedTest and receive its data from the data() method. This ensures proper test isolation for each parameter set.
| import org.junit.jupiter.params.provider.MethodSource; | ||
|
|
||
| @RunWith(Parameterized.class) | ||
| public class BigQueryArrowArrayOfPrimitivesTest { |
There was a problem hiding this comment.
Removing the @RunWith(Parameterized.class) annotation is a necessary step for JUnit 5 migration.
| private final int javaSqlTypeCode; | ||
| private Array array; | ||
| private final StandardSQLTypeName currentType; | ||
| @RegisterExtension public static final TimeZoneRule timeZoneRule = new TimeZoneRule("UTC"); |
There was a problem hiding this comment.
Migrating from @ClassRule to @RegisterExtension for static rules is the correct approach in JUnit 5.
| import org.junit.jupiter.params.provider.MethodSource; | ||
|
|
||
| @RunWith(Parameterized.class) | ||
| public class BigQueryDefaultCoercionsTest { |
There was a problem hiding this comment.
Removing the @RunWith(Parameterized.class) annotation is a necessary step for JUnit 5 migration.
| public void beforeAll(ExtensionContext context) { | ||
| defaultTimeZone = TimeZone.getDefault(); | ||
| TimeZone.setDefault(TimeZone.getTimeZone(timeZoneId)); | ||
| } |
There was a problem hiding this comment.
The beforeAll method correctly sets the default time zone for all tests in the class, storing the original time zone to restore it later.
| } | ||
|
|
||
| @Override | ||
| public void afterAll(ExtensionContext context) { |
There was a problem hiding this comment.
The afterAll method correctly restores the original default time zone after all tests in the class have run.
| public void beforeEach(ExtensionContext context) { | ||
| if (defaultTimeZone == null) { | ||
| defaultTimeZone = TimeZone.getDefault(); | ||
| } | ||
| TimeZone.setDefault(TimeZone.getTimeZone(timeZoneId)); |
There was a problem hiding this comment.
The beforeEach method correctly sets the default time zone before each test method, ensuring test isolation regarding time zone settings.
| @Override | ||
| public Statement apply(Statement base, Description description) { | ||
| return new Statement() { | ||
| @Override | ||
| public void evaluate() throws Throwable { | ||
| try { | ||
| TimeZone.setDefault(TimeZone.getTimeZone(timeZoneId)); | ||
| base.evaluate(); | ||
| } finally { | ||
| TimeZone.setDefault(defaultTimeZone); | ||
| } | ||
| } | ||
| }; | ||
| public void afterEach(ExtensionContext context) { | ||
| if (defaultTimeZone != null) { | ||
| TimeZone.setDefault(defaultTimeZone); | ||
| } |
There was a problem hiding this comment.
The afterEach method correctly restores the original default time zone after each test method, ensuring test isolation regarding time zone settings.
2 participants
No description provided.