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

Issue #6916: migrate tests to junit5 for annotation package #7303

Merged
merged 1 commit into from Dec 6, 2019

Conversation

pbludov
Copy link
Member

@pbludov pbludov commented Dec 3, 2019

Issue #6916

Auto converted to Junit5 with IDEA inspections.
As a side effect, some imports replaced with static imports.

@pbludov pbludov requested a review from rnveach December 3, 2019 19:11
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

I am fine with static import in tests.

item

<subpackage name="checks">
<subpackage name="annotation">
<allow pkg="org.junit.jupiter.api"/>
<disallow pkg="org.junit"/>
Copy link
Member

Choose a reason for hiding this comment

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

does it make sense to do this on top level, and explicitly allow junit in packages that are not converted yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is a temporary config. I will add processed packages here so that no one accidentally adds a junit4 test during the migration. Once all tests will use juni5, this section will be removed. Junit4 package annotations will not be available because the junit4 module will be removed from the pom.xml and this section will not be needed any more.

Copy link
Member

Choose a reason for hiding this comment

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

I meant something like:
https://github.com/checkstyle/checkstyle/blob/master/config/import-control.xml#L15
https://github.com/checkstyle/checkstyle/blob/master/config/import-control.xml#L70

and it will works same during migration, and in each commit/PR , you will remove allow for certain package so so at the end config will self cleanup.

Copy link
Member Author

Choose a reason for hiding this comment

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

done. Now Junit4 globally forbidden and explicitly enabled for every test except tests in checks/annotation package. During migration, the allowed package list will shrink until it is empty.

@pbludov
Copy link
Member Author

pbludov commented Dec 3, 2019

IDEA inspections fails to recognize assertArrayEquals method as an assert method:

https://teamcity.jetbrains.com/viewLog.html?buildId=2660367&tab=Inspection&buildTypeId=Checkstyle_IdeaInspectionsPullRequest

It's upsetting. I'll try to use Google Truth assertions instead.

@romani
Copy link
Member

romani commented Dec 3, 2019

try to remove static import and help IDEA to recognize it.
and please recheck that inspection config should be updated:
image

I'll try to use Google Truth assertions instead.

it is better to no overcomplicate PRs with extra update.

@rnveach
Copy link
Member

rnveach commented Dec 3, 2019

@pbludov @romani It is more concerning that pitest does not like junit5. This may be a bigger deal breaker than TC complaining.

https://circleci.com/gh/pbludov/checkstyle/2995?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Caused by: org.apache.maven.plugin.MojoFailureException: Mutation score of 0 is below threshold of 100

Pitest says we lost all coverage.

Looks like we might need a new dependency for pitest. https://github.com/pitest/pitest-junit5-plugin

@pbludov
Copy link
Member Author

pbludov commented Dec 4, 2019

Looks like we might need a new dependency for pitest. https://github.com/pitest/pitest-junit5-plugin

It works, thank you.

@pbludov
Copy link
Member Author

pbludov commented Dec 4, 2019

please recheck that inspection config should be updated:

Done.

@pbludov pbludov requested a review from romani December 4, 2019 09:24
@pbludov pbludov assigned romani and unassigned pbludov Dec 4, 2019
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

last item:

config/import-control-test.xml Outdated Show resolved Hide resolved
@romani romani assigned pbludov and unassigned romani Dec 4, 2019
@pbludov pbludov assigned romani and unassigned pbludov Dec 5, 2019
@pbludov pbludov requested a review from romani December 5, 2019 02:38
@rnveach
Copy link
Member

rnveach commented Dec 5, 2019

Shippable still hasn't finished. TC can be ignored.

@romani
Copy link
Member

romani commented Dec 5, 2019

It is better to not ignore, but recheck that violations are same as in master

@pbludov pbludov assigned pbludov and unassigned romani Dec 5, 2019
@pbludov pbludov merged commit 4ab0354 into checkstyle:master Dec 6, 2019
@pbludov pbludov deleted the issue-6916-checks-ann branch December 6, 2019 05:17
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