FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

chore: Migrate Junit 4 to Junit 5 for showcase by zhumin8 · Pull Request #2757 · googleapis/sdk-platform-java · GitHub

This repository was archived by the owner on May 14, 2026. It is now read-only.
/ sdk-platform-java Public archive

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

Merged
lqiu96 merged 20 commits into
mainfrom
junit-migrate
May 20, 2024
ConversationCommits (20)ChecksFiles changed
Merged

chore: Migrate Junit 4 to Junit 5 for showcase#2757
lqiu96 merged 20 commits into
mainfrom
junit-migrate

Conversation

zhumin8 commented May 9, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 Bot added the size: xl Pull request size is extra large. label May 9, 2024
zhumin8 added the owlbot:run Add this label to trigger the Owlbot post processor. label May 9, 2024
gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 9, 2024
zhumin8 marked this pull request as ready for review May 9, 2024 08:46
zhumin8 requested review from a team, blakeli0 and lqiu96 May 9, 2024 08:46
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 mentioned this pull request Apr 1, 2026
Open
lqiu96 approved these changes May 10, 2024
View reviewed changes

lqiu96 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

lqiu96 commented May 17, 2024

Copy link
Copy Markdown
Member

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

blakeli0 reviewed May 17, 2024
View reviewed changes
Comment thread showcase/gapic-showcase/pom.xml Outdated

lqiu96 commented May 17, 2024

Copy link
Copy Markdown
Member 
[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 commented May 17, 2024

Copy link
Copy Markdown
Member

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 reviewed May 17, 2024
View reviewed changes
Comment thread 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>

lqiu96 May 17, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

sonarqubecloud Bot commented May 17, 2024

Copy link
Copy Markdown

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

sonarqubecloud Bot commented May 17, 2024

Copy link
Copy Markdown

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 reviewed May 17, 2024
View reviewed changes
Comment thread showcase/pom.xml
<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>

blakeli0 May 17, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

lqiu96 May 20, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 merged commit 5799827 into main May 20, 2024
lqiu96 deleted the junit-migrate branch May 20, 2024 14:17
lqiu96 added a commit that referenced this pull request May 22, 2024
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers
lqiu96 lqiu96 approved these changes
blakeli0 blakeli0 approved these changes
JoeWang1127 JoeWang1127 approved these changes
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL