Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

test: migrate GetBinaryFilesTest to JUnit 5 #4486

Merged
merged 3 commits into from
Jan 14, 2022

Conversation

MartinWitt
Copy link
Collaborator

#3919

Change Log

The following bad smells are refactored:

JUnit4-@test

The JUnit 4 @Test annotation should be replaced with JUnit 5 @Test annotation.

JUnit4Assertion

The JUnit4 assertion should be replaced with JUnit5 Assertions.

The following has changed in the code:

JUnit4-@test

  • Replaced junit 4 test annotation with junit 5 test annotation in testSingleBinary
  • Replaced junit 4 test annotation with junit 5 test annotation in testExistingButNotBuiltBinary
  • Replaced junit 4 test annotation with junit 5 test annotation in testMultiClassInSingleFile
  • Replaced junit 4 test annotation with junit 5 test annotation in testNestedTypes
  • Replaced junit 4 test annotation with junit 5 test annotation in testAnonymousClasses

JUnit4Assertion

  • Transformed junit4 assert to junit 5 assertion in testSingleBinary
  • Transformed junit4 assert to junit 5 assertion in testExistingButNotBuiltBinary
  • Transformed junit4 assert to junit 5 assertion in testMultiClassInSingleFile
  • Transformed junit4 assert to junit 5 assertion in testNestedTypes
  • Transformed junit4 assert to junit 5 assertion in testAnonymousClasses

 The following has changed in the code:
Replaced junit 4 test annotation with junit 5 test annotation in testSingleBinary
Replaced junit 4 test annotation with junit 5 test annotation in testExistingButNotBuiltBinary
Replaced junit 4 test annotation with junit 5 test annotation in testMultiClassInSingleFile
Replaced junit 4 test annotation with junit 5 test annotation in testNestedTypes
Replaced junit 4 test annotation with junit 5 test annotation in testAnonymousClasses
Transformed junit4 assert to junit 5 assertion in testSingleBinary
Transformed junit4 assert to junit 5 assertion in testExistingButNotBuiltBinary
Transformed junit4 assert to junit 5 assertion in testMultiClassInSingleFile
Transformed junit4 assert to junit 5 assertion in testNestedTypes
Transformed junit4 assert to junit 5 assertion in testAnonymousClasses
Copy link
Collaborator

@slarse slarse left a comment

Choose a reason for hiding this comment

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

Weird annotation.

@TempDir
Path tmpFolder;

@BeforeEach
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks strange?

Copy link
Collaborator Author

@MartinWitt MartinWitt Jan 14, 2022

Choose a reason for hiding this comment

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

Sadly, JUnit 5 has no longer the @Rule annotations but extensions, see https://junit.org/junit5/docs/current/user-guide/#migrating-from-junit4-rule-support. The replacement for the old temporary folder concept is https://junit.org/junit5/docs/5.4.2/api/org/junit/jupiter/api/io/TempDir.html, https://www.baeldung.com/junit-5-temporary-directory.
Personally, I hate the new temporary folder concept and hope someone writes a clean extension for it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe I'm dense here, but where does @BeforeEach come into the picture?

I actuall quite like the new @TempDir construct, allows you to remove the field (ugh, fields!) and do something like this:

@Test
void testSomething(@TempDir workdir) {
    // use injected temporary directory
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh the @BeforeEach should be removed you are right

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actuall quite like the new @tempdir construct, allows you to remove the field (ugh, fields!) and do something like this:

Yes, if you use this style it's nice, but then the whole codebase should do it this way and not mix of different styles

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, if you use this style it's nice, but then the whole codebase should do it this way and not mix of different styles

Good practices have to start somewhere :). That's kind of the thing with legacy, good practices have to be adopted gradually. And often, they are never fully adopted, but we can be watchful for future PRs.

If someone were to submit a PR with a JUnit5 test that had a field like this, I'd request a change. In this case, it's already there, so it's ok.

@slarse slarse merged commit ab769db into INRIA:master Jan 14, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants