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.

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

test: migrate client module bookie tests to junit 5 #4359

Merged

Conversation

sherlock-lin
Copy link
Contributor

@sherlock-lin sherlock-lin commented May 11, 2024

Descriptions of the changes in this PR:
Main Issue: #4322


  • Make sure tests pass via mvn clean apache-rat:check install spotbugs:check.

@sherlock-lin sherlock-lin changed the title Upgrading Unit Tests to Junit 5 Upgrade Unit Tests to Junit 5 May 11, 2024
@sherlock-lin
Copy link
Contributor Author

@shoothzj

@sherlock-lin sherlock-lin force-pushed the sherlock-upgrading-to-junit5-0511 branch from a22f04e to ec09b5f Compare May 11, 2024 08:49
Copy link
Member

@shoothzj shoothzj left a comment

Choose a reason for hiding this comment

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

Looks good, I have two advice:

  • I think migrate might be better than Upgrade, WDYT?
  • As we have many tests, we should point on which part of tests are migrated in PR title. It's better to do like Migrate All client tests to junit5, or Migrate xxxx module tests to junit5

@sherlock-lin sherlock-lin changed the title Upgrade Unit Tests to Junit 5 Migrate Unit Tests to Junit 5 May 11, 2024
@sherlock-lin sherlock-lin changed the title Migrate Unit Tests to Junit 5 Migrate client module about bookie tests to Junit 5 May 11, 2024
@sherlock-lin sherlock-lin changed the title Migrate client module about bookie tests to Junit 5 Migrate client module about bookie tests to junit 5 May 11, 2024
@sherlock-lin
Copy link
Contributor Author

Looks good, I have two advice:

  • I think migrate might be better than Upgrade, WDYT?
  • As we have many tests, we should point on which part of tests are migrated in PR title. It's better to do like Migrate All client tests to junit5, or Migrate xxxx module tests to junit5

That's good advice, I'll make the adjustments!

@@ -35,6 +37,7 @@ public BookieRecoveryUseIOThreadTest() {
super(1);
}

@BeforeEach
Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need this annotation, it's already on the base BookKeeperClusterTestCase

@shoothzj
Copy link
Member

Thanks for your contribution, please fix the checkstyle violations. :)

@sherlock-lin sherlock-lin force-pushed the sherlock-upgrading-to-junit5-0511 branch 3 times, most recently from 40a24e9 to 1289978 Compare May 15, 2024 03:11
@sherlock-lin sherlock-lin force-pushed the sherlock-upgrading-to-junit5-0511 branch from 1289978 to 44ce835 Compare May 15, 2024 06:17
@sherlock-lin
Copy link
Contributor Author

Thanks for your contribution, please fix the checkstyle violations. :)

ok

@@ -35,7 +35,8 @@
import org.apache.bookkeeper.meta.ZkLedgerUnderreplicationManager;
import org.apache.bookkeeper.net.BookieId;
import org.apache.bookkeeper.test.BookKeeperClusterTestCase;
import org.junit.Test;
import org.junit.jupiter.api.Test;

Copy link
Member

Choose a reason for hiding this comment

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

nit: unnecessary blank line

@@ -20,8 +20,9 @@
*/
package org.apache.bookkeeper.client;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.fail;

Copy link
Member

Choose a reason for hiding this comment

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

nit: unnecessary blank line

@@ -61,6 +61,7 @@ public class BookieRecoveryTest extends BookKeeperClusterTestCase {

// Object used for synchronizing async method calls
class SyncObject {

Copy link
Member

Choose a reason for hiding this comment

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

nit: unnecessary blank line

import org.junit.Assert;
import org.junit.Test;
import org.junit.jupiter.api.Test;

Copy link
Member

Choose a reason for hiding this comment

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

nit: unnecessary blank line

@sherlock-lin sherlock-lin force-pushed the sherlock-upgrading-to-junit5-0511 branch from 44ce835 to aee1cd3 Compare May 16, 2024 06:15
@shoothzj shoothzj changed the title Migrate client module about bookie tests to junit 5 test: migrate client module bookie tests to junit 5 May 16, 2024
@shoothzj shoothzj merged commit 2c62567 into apache:master May 16, 2024
21 checks passed
@shoothzj
Copy link
Member

Thanks for your contribution, and welcome to the community. :)

@sherlock-lin
Copy link
Contributor Author

Thanks for your contribution, and welcome to the community. :)

Glad to join the community. 😃

@sherlock-lin sherlock-lin deleted the sherlock-upgrading-to-junit5-0511 branch May 16, 2024 07:47
@shoothzj shoothzj self-assigned this May 26, 2024
@shoothzj shoothzj added this to the 4.18.0 milestone May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants