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.

Sign up for GitHub

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

Jump to bottom

Issue #10932: remove dependency on Powermock from tests #10947

Merged
merged 1 commit into from Dec 20, 2021
Merged

Issue #10932: remove dependency on Powermock from tests #10947

merged 1 commit into from Dec 20, 2021

Conversation

pbludov
Copy link
Member

@pbludov pbludov commented Nov 13, 2021 •
edited

Issue #10932

Closes #6439 and #7368

  • All tests now compatible with JDK17, no need for illegal-access=warn workaround for JD16

@pbludov pbludov self-assigned this Nov 13, 2021
@pbludov pbludov force-pushed the issue-10932-mock-static branch 2 times, most recently from e5b6aca to 380bb4c Compare November 13, 2021 21:13
@romani
Copy link
Member

romani commented Nov 14, 2021

Your PR is huge, please consider to split.

@pbludov pbludov force-pushed the issue-10932-mock-static branch 6 times, most recently from 1aac646 to da467fa Compare November 14, 2021 07:28
@pbludov
Copy link
Member Author

pbludov commented Nov 14, 2021

Your PR is huge, please consider to split.

Sure. We should start with the hardest case: it is not possible to mock methods in the System class.
We wave a test that mocks System.exit as a workaround for jacoco/jacoco#117

What should we do?

@nrmancuso
Copy link
Member

@pbludov split PR's from this one will also close #9987, #9325, #10053, and possibly other pitest issues, please confirm this.

@pbludov
Copy link
Member Author

pbludov commented Nov 29, 2021

@pbludov split PR's from this one will also close #9987, #9325, #10053, and possibly other pitest issues, please confirm this.

Yes, all related Pitest issues should be closed/updated.

@pbludov pbludov force-pushed the issue-10932-mock-static branch 5 times, most recently from 4c50c48 to c8aee44 Compare December 2, 2021 10:14
@pbludov pbludov mentioned this pull request Dec 4, 2021
Closed
@rnveach
Copy link
Member

rnveach commented Dec 10, 2021

Looking at pitest report, why can't PR be split between the different groupings? treewalker, utils, commons, headers, ...

@pbludov
Copy link
Member Author

pbludov commented Dec 12, 2021 •
edited

Looking at pitest report, why can't PR be split between the different groupings? treewalker, utils, commons, headers, ...

Because Mockito.mockstatic cannot coexist with PowerMockito.mockstatic. It is necessary to switch all PowerMockito methods to their Mockito equivalent in one PR.

@nrmancuso
Copy link
Member

All tests now compatible with JDK17, no need for illegal-access=warn workaround for JD16

Does this mean that we can remove entire profile for:

checkstyle/pom.xml

Line 1907 in e4cf277

<surefire.options>--illegal-access=warn</surefire.options>

?

@pbludov
Copy link
Member Author

pbludov commented Dec 12, 2021

Does this mean that we can remove entire profile for:

Yes. And JUnit4 dependency too. In the next PR.

romani
romani requested changes Dec 13, 2021
View reviewed changes
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.

items:

src/test/java/com/puppycrawl/tools/checkstyle/PropertyCacheFileTest.java Outdated Show resolved Hide resolved
src/test/java/com/puppycrawl/tools/checkstyle/TreeWalkerTest.java Outdated Show resolved Hide resolved
src/test/java/com/puppycrawl/tools/checkstyle/TreeWalkerTest.java Outdated Show resolved Hide resolved
src/test/java/com/puppycrawl/tools/checkstyle/utils/CommonUtilTest.java Outdated Show resolved Hide resolved
romani
romani requested changes Dec 13, 2021
View reviewed changes
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.

items:

src/main/java/com/puppycrawl/tools/checkstyle/utils/CommonUtil.java Outdated Show resolved Hide resolved
@pbludov
Copy link
Member Author

pbludov commented Dec 14, 2021

GitHub, rebase

romani
romani requested changes Dec 17, 2021
View reviewed changes
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.

item:

src/test/java/com/puppycrawl/tools/checkstyle/AuditEventDefaultFormatterTest.java Outdated
public void testFormatModuleNameContainsCheckSuffix() {
final AuditEvent mock = mock(AuditEvent.class);
when(mock.getSourceName()).thenReturn("TestModuleCheck");
when(mock.getSeverityLevel()).thenReturn(SeverityLevel.WARNING);
Copy link
Member

Choose a reason for hiding this comment

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

why we do mock if we can just create AuditEvent with required values ?

Copy link
Member Author

Choose a reason for hiding this comment

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

To cover this line we need a class that has no package and ends with Check suffix.

This can be done by adding a class to the test source root.

Or, by extracting this code to some utility method.
Such method can be reused here, here, here and possible in many other methods.

Copy link
Member

Choose a reason for hiding this comment

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

This can be done by adding a class to the test source root.

Let's do this.
Without such comment, it is very very unclear.
Previously we agreed that if we do some reflection based test we explain in comment what is a goal.
I think mocking tests should be commented also. Ideally no mocking tests.

Copy link
Member

Choose a reason for hiding this comment

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

I see comment over methods, but I would be awesome to avoid mocking if that is simple as you describe.

I am approving PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please review #11056

@pbludov pbludov force-pushed the issue-10932-mock-static branch 2 times, most recently from b1a6359 to 2a98d5c Compare December 18, 2021 08:09
romani
romani approved these changes Dec 18, 2021
View reviewed changes
Copy link
Member

@romani romani left a comment •
edited

Choose a reason for hiding this comment

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

Ok to merge.

please see comment above as last polishing before next reviewer

@romani romani assigned rnveach and unassigned romani Dec 18, 2021
@romani romani requested a review from rnveach December 18, 2021 15:23
@pbludov pbludov marked this pull request as draft December 19, 2021 09:04
@pbludov
Copy link
Member Author

pbludov commented Dec 19, 2021 •
edited

Blocked by #11056

@pbludov pbludov marked this pull request as ready for review December 20, 2021 08:20
@pbludov
Copy link
Member Author

pbludov commented Dec 20, 2021

@rnveach please review this PR

@rnveach rnveach merged commit 8129a4f into checkstyle:master Dec 20, 2021
This was referenced Dec 20, 2021
Closed
Merged
@nrmancuso nrmancuso mentioned this pull request Dec 20, 2021
Closed
@pbludov pbludov deleted the issue-10932-mock-static branch December 22, 2021 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romani romani romani approved these changes

@rnveach rnveach rnveach approved these changes

@strkkk strkkk strkkk approved these changes

Assignees

@rnveach rnveach

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove powermock
5 participants
@pbludov @romani @nrmancuso @rnveach @strkkk