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

SNOW-1693588 Upgrade to JUnit5 #1909

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

SNOW-1693588 Upgrade to JUnit5 #1909

wants to merge 21 commits into from
+5,245 −4,676

Conversation

sfc-gh-astachowski
Copy link
Collaborator

@sfc-gh-astachowski sfc-gh-astachowski commented Oct 4, 2024 •
edited
Loading

Overview

SNOW-1693588 upgraded all tests to junit5. There are a lot of changes, but most are of following types:

  • Changed imports to junit5
  • Changed annotations to their 1:1 counterparts ( @Before to @BeforeEach, @BeforeClass to @BeforeAll etc.)
  • Changed parametrisation from junit4 style (class level) to junit5 style (method level)
  • Changed temporary folders from junit4 style to junit5 style (@Rule to @TempDir)
  • Changed conditional enable/disable annotations to junit5 style

Pre-review self checklist

  • PR branch is updated with all the changes from master branch
  • The code is correctly formatted (run mvn -P check-style validate)
  • New public API is not unnecessary exposed (run mvn verify and inspect target/japicmp/japicmp.html)
  • The pull request name is prefixed with SNOW-XXXX:
  • Code is in compliance with internal logging requirements

@sfc-gh-astachowski sfc-gh-astachowski requested a review from a team as a code owner October 4, 2024 08:58
sfc-gh-dprzybysz
sfc-gh-dprzybysz reviewed Oct 9, 2024
View reviewed changes
parent-pom.xml Outdated
@@ -59,7 +59,8 @@
<jna.version>5.13.0</jna.version>
<joda.time.version>2.8.1</joda.time.version>
<json.smart.version>2.4.9</json.smart.version>
<junit.version>4.13.2</junit.version>
<junit.version>5.11.1</junit.version>
<junit4.version>4.13.2</junit4.version>
Copy link
Collaborator

Choose a reason for hiding this comment

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

in parent pom we don't neet junit4 anymore?

parent-pom.xml Outdated
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>${junit.version}</version>
<version>${junit4.version}</version>
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we remove junit 4?

parent-pom.xml
@@ -257,10 +258,28 @@
<artifactId>opencensus-api</artifactId>
<version>${opencensus.version}</version>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
Copy link
Collaborator

Choose a reason for hiding this comment

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

we are chaning the library - please start the security review for the new artifacts if not started

src/test/java/net/snowflake/client/ConditionalIgnoreRule.java Outdated
@@ -26,6 +26,7 @@
import org.junit.runners.model.FrameworkMethod;
import org.junit.runners.model.Statement;

@Deprecated
Copy link
Collaborator

Choose a reason for hiding this comment

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

we need to keep the depracated methods for the old driver tests? it's not our API so can we remove it even now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We probably could. I recently noticed that some of their static methods are being used in assumptions, so I'll investigate what can be done about this (worst case we can extract the static methods to a util class)

src/test/java/net/snowflake/client/annotations/DontRunOnGithubActions.java Show resolved Hide resolved
src/test/java/net/snowflake/client/jdbc/structuredtypes/ResultSetStructuredTypesLatestIT.java Outdated
@@ -39,81 +38,67 @@
import net.snowflake.client.jdbc.structuredtypes.sqldata.NullableFieldsSqlData;
import net.snowflake.client.jdbc.structuredtypes.sqldata.SimpleClass;
import net.snowflake.client.jdbc.structuredtypes.sqldata.StringClass;
import org.junit.After;
import net.snowflake.client.providers.FormatProvider;
import org.junit.Assume;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about Assume usage? should we replace it with junit 5 equivalent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed we should, good catch

src/test/java/net/snowflake/client/providers/BooleanProvider.java
@@ -0,0 +1,13 @@
package net.snowflake.client.providers;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add missing copyright header to the new files

src/test/java/net/snowflake/client/providers/FormatProvider.java Outdated
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.ArgumentsProvider;

public class FormatProvider implements ArgumentsProvider {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's rename to ResultFormatProvider

src/test/java/net/snowflake/client/providers/SimpleFormatProvider.java
import org.junit.jupiter.api.extension.ExtensionContext;
import org.junit.jupiter.params.provider.Arguments;

public class SimpleFormatProvider extends SnowflakeArgumentsProvider {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought that it simple format for data time :) - can we rename this class to SimpleResultFormatProvider?

src/test/java/net/snowflake/client/providers/TimezoneProvider.java

@Override
protected List<Arguments> rawArguments(ExtensionContext context) {
return timeZones.subList(0, length);
Copy link
Collaborator

Choose a reason for hiding this comment

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

we need length to just copy the list? cannot we just keep unmodifiable list in this class?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea here was to allow for some tests to easily get just a few of this. There were some long running tests that were originally running with 3-4 timezones only, and I decided not to double their execution time. Having said that, this is not useful when using it directly in the @ArgumentsSource annotation, only when using the cartesianProduct util

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-dprzybysz sfc-gh-dprzybysz sfc-gh-dprzybysz left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sfc-gh-astachowski @sfc-gh-dprzybysz