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

chore: Migrate Junit 4 to Junit 5 for showcase #2757

Merged
merged 20 commits into from May 20, 2024
Merged

chore: Migrate Junit 4 to Junit 5 for showcase #2757

merged 20 commits into from May 20, 2024

Conversation

zhumin8
Copy link
Contributor

@zhumin8 zhumin8 commented May 9, 2024 •
edited

fixes #2728, attempt to remove Junit 4 support after migration.
Other than POM dependency migrate, changes include:

  • package name changes
  • Junit 5 syntax upgrades, e.g. @Before --> @BeforeEach, Replace assertion methods
  • remove public modifier on tests and test classes.
  • Refactor JUnit 4 TemporaryFolder @Rule in ITGdch.java to JUnit 5 @TempDir
  • Replace @Test(timeout = 15000L) in ITClientShutdown.java with @Timeout(15)
  • Update @RunWith(Parameterized.class) test in ITHttpAnnotation.java to @ParameterizedTest with @MethodSource("data")

Note: #2737 creates a new test class with JUnit4 syntax. Depending on merging order, I will either update in this pr, or #2737. Updated.

Due to truth library depending on junit 4 (see issue), junit 4 cannot be completely removed, or will encounter java.lang.ClassNotFoundException: org.junit.runner.notification.RunListener running tests with maven surefire. To keep things cleaner, excluding the implicitly junit brought in from truth and junit-vintage-engine. We could also do the reverse, and make a comment if that's prefered.

@product-auto-label product-auto-label bot added the size: xl Pull request size is extra large. label May 9, 2024
@zhumin8 zhumin8 added the owlbot:run Add this label to trigger the Owlbot post processor. label May 9, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 9, 2024
@zhumin8 zhumin8 marked this pull request as ready for review May 9, 2024 08:46
@zhumin8 zhumin8 requested a review from a team as a code owner May 9, 2024 08:46
@zhumin8 zhumin8 requested review from blakeli0 and lqiu96 May 9, 2024 08:46
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: xl Pull request size is extra large. labels May 10, 2024
@zhumin8 zhumin8 mentioned this pull request May 10, 2024
Open
lqiu96
lqiu96 approved these changes May 10, 2024
View reviewed changes
Copy link
Contributor

@lqiu96 lqiu96 left a comment

Choose a reason for hiding this comment

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

LGTM. Added a few nits. If you could test them locally and see if they work.

@lqiu96
Copy link
Contributor

lqiu96 commented May 17, 2024

Since Min is OOO, I will take a look a final few things for this issue and merge if everything else looks fine.

@lqiu96
Copy link
Contributor

lqiu96 commented May 17, 2024 

[INFO] --- maven-failsafe-plugin:3.2.5:integration-test (default) @ gapic-showcase ---
[INFO] Using configured provider org.apache.maven.surefire.junitcore.JUnitCoreProvider
[INFO] 
[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 0, Failures: 0, Errors: 0, Skipped: 0

Looks like the ITs aren't actually running.

@lqiu96
Copy link
Contributor

lqiu96 commented May 17, 2024

I see it is running the tests now:

[INFO] --- clirr-maven-plugin:2.8:check (default) @ gapic-showcase ---
[INFO] Skipping execution
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for GAPIC Showcase Client Core Parent 0.0.1-SNAPSHOT:
[INFO] 
[INFO] GAPIC Showcase Client Core Parent .................. SUCCESS [  2.109 s]
[INFO] proto-gapic-showcase-v1beta1 ....................... SUCCESS [  1.008 s]
[INFO] grpc-gapic-showcase-v1beta1 ........................ SUCCESS [  2.228 s]
[INFO] GAPIC Showcase Client .............................. SUCCESS [01:39 min]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:45 min
[INFO] Finished at: 2024-05-17T19:11:27Z
[INFO] ------------------------------------------------------------------------

lqiu96
lqiu96 reviewed May 17, 2024
View reviewed changes
showcase/pom.xml
Comment on lines +57 to +61
<groupId>org.junit</groupId>
<artifactId>junit-bom</artifactId>
<version>5.10.2</version>
<type>pom</type>
<scope>import</scope>
Copy link
Contributor

Choose a reason for hiding this comment

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

As this doesn't inherit from shared-deps or pom-parent, we declare the version here.

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 17, 2024

Quality Gate Passed Quality Gate passed for 'gapic-generator-java-root'

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 17, 2024

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

blakeli0
blakeli0 reviewed May 17, 2024
View reviewed changes
showcase/pom.xml
@@ -116,6 +117,15 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-failsafe-plugin</artifactId>
<!-- Shared-Configs defines JUnit Provider 4.7 as default: https://github.com/googleapis/java-shared-config/blob/465bb399aef9aa8383f11c23f10a97df49c1d057/java-shared-config/pom.xml#L86-L92 -->
<!-- Override it here for showcase only as other libraries may still be reliant on JUnit4 -->
<dependencies>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we know why the integration tests would not run without this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it's due the failsafe using junit-provider 4.7 (which works for junit 4.7+) as the selected provider instead of junit-platform: https://maven.apache.org/surefire/maven-failsafe-plugin/examples/junit-platform.html#provider-selection

@lqiu96 lqiu96 merged commit 5799827 into main May 20, 2024
33 checks passed
@lqiu96 lqiu96 deleted the junit-migrate branch May 20, 2024 14:17
lqiu96 added a commit that referenced this pull request May 22, 2024
@zhumin8 @burkedavison @lqiu96
chore: Migrate Junit 4 to Junit 5 for showcase (#2757)
fd91c65 
fixes #2728, attempt to remove Junit 4 support after migration.
Other than POM dependency migrate, changes include:
- package name changes
- Junit 5 syntax upgrades, e.g. `@Before` --> `@BeforeEach`, Replace
assertion methods
- remove public modifier on tests and test classes.
- Refactor JUnit 4 TemporaryFolder `@Rule` in
[ITGdch.java](https://github.com/googleapis/sdk-platform-java/pull/2757/files#diff-6ae7755a0b038e1a2febae2d27e36c762f6751b8c7db577421667069399884b4)
to JUnit 5 `@TempDir`
- Replace `@Test(timeout = 15000L)` in
[ITClientShutdown.java](https://github.com/googleapis/sdk-platform-java/pull/2757/files#diff-70d1df57471178a7a63302f82e4a4855ffbbd642ea67d92d501bd1f7008957ca)
with `@Timeout(15)`
- Update `@RunWith(Parameterized.class)` test in
[ITHttpAnnotation.java](https://github.com/googleapis/sdk-platform-java/pull/2757/files#diff-03d420650ecc9fe78ad4887761043c4fdceaa978f464ce30cfc4ed5f8be9b64d)
to `@ParameterizedTest` with `@MethodSource("data")`

~~Note: #2737 creates a new test class with JUnit4 syntax. Depending on
merging order, I will either update in this pr, or #2737.~~ Updated.


Due to truth library depending on junit 4 ([see
issue](google/truth#333)), junit 4 cannot be
completely removed, or will encounter `java.lang.ClassNotFoundException:
org.junit.runner.notification.RunListener` running tests with maven
surefire. To keep things cleaner, excluding the implicitly junit brought
in from truth and `junit-vintage-engine`. We could also do the reverse,
and make a comment if that's prefered.

---------

Co-authored-by: Burke Davison <40617934+burkedavison@users.noreply.github.com>
Co-authored-by: Lawrence Qiu <lawrenceqiu@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lqiu96 lqiu96 lqiu96 approved these changes

@blakeli0 blakeli0 blakeli0 approved these changes

@JoeWang1127 JoeWang1127 JoeWang1127 approved these changes

Assignees
No one assigned
Labels
size: l Pull request size is large.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate Junit 4 to Junit 5: showcase
5 participants
@zhumin8 @lqiu96 @blakeli0 @JoeWang1127 @burkedavison