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

GH-42002: [Java] Update Unit Tests for Vector Module #42019

Merged
merged 7 commits into from
Jun 7, 2024

Conversation

llama90
Copy link
Contributor

@llama90 llama90 commented Jun 6, 2024

Rationale for this change

Update package from JUnit 4(org.junit) to JUnit 5(org.junit.jupiter).

What changes are included in this PR?

  • Replacing org.junit with org.junit.jupiter.api.
  • Updating Assertions.assertXXX to assertXXX using static imports.
  • Updating annotations such as @Before, @BeforeClass, @After, @AfterClass.
    • @Before -> @BeforeEach
    • @BeforeClass -> @BeforeAll
    • @After -> @AfterEach
    • @AfterClass -> @AfterAll
    • @Test -> @Test with org.junit.jupiter
  • Removing unused @Rule Annotation
  • Updating Parameterized test
  • Doing self review

Are these changes tested?

Yes, existing tests have passed.

Are there any user-facing changes?

No.

@llama90 llama90 requested a review from lidavidm as a code owner June 6, 2024 17:12
Copy link

github-actions bot commented Jun 6, 2024

⚠️ GitHub issue #42002 has been automatically assigned in GitHub to PR creator.

@vibhatha
Copy link
Collaborator

vibhatha commented Jun 6, 2024

@llama90 I will review this later today.

@llama90
Copy link
Contributor Author

llama90 commented Jun 6, 2024

@vibhatha Thanks! I'll try to review it myself before your review.

@vibhatha
Copy link
Collaborator

vibhatha commented Jun 6, 2024

@llama90 wonderful! Ping me once you're ready. Thanks.

Copy link
Contributor Author

@llama90 llama90 left a comment

Choose a reason for hiding this comment

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

@vibhatha Hello.

I have completed my self-review. I specifically focused on the ParameterizedTest and Rule annotations. Please prioritize reviewing these sections.

Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ParameterizedTest

Copy link
Collaborator

Choose a reason for hiding this comment

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

Approach LGTM! Thanks for the improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ParameterizedTest

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, this one too. Thanks.

@@ -154,9 +152,6 @@ public void testSchemaDictionaryMessageSerialization() throws IOException {
assertEquals(schema, deserialized);
}

@Rule
public ExpectedException expectedEx = ExpectedException.none();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this code really unused?

Copy link
Member

Choose a reason for hiding this comment

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

The test that used it was added 7 years ago and since removed, so it appears so.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jun 7, 2024
Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -154,9 +152,6 @@ public void testSchemaDictionaryMessageSerialization() throws IOException {
assertEquals(schema, deserialized);
}

@Rule
public ExpectedException expectedEx = ExpectedException.none();
Copy link
Member

Choose a reason for hiding this comment

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

The test that used it was added 7 years ago and since removed, so it appears so.

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jun 7, 2024
Copy link
Collaborator

@vibhatha vibhatha left a comment

Choose a reason for hiding this comment

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

Thanks @llama90 for working on this PR. LGTM!

@llama90
Copy link
Contributor Author

llama90 commented Jun 7, 2024

Thank you for your reviews @vibhatha @lidavidm

@lidavidm lidavidm merged commit fe4d04f into apache:main Jun 7, 2024
19 checks passed
@lidavidm lidavidm removed the awaiting merge Awaiting merge label Jun 7, 2024
Copy link

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit fe4d04f.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants