-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
minor: solved pitest issues with ImportOrderCheck #6424
Conversation
src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportOrderCheck.java
Outdated
Show resolved
Hide resolved
src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportOrderCheck.java
Outdated
Show resolved
Hide resolved
Let me know if you want me to assign this to issue I mentioned in first post or you want this to reference the PR itself. |
9fa0b55
to
21e802f
Compare
Time dropped now by 20 minutes.
|
I tried testing something out by just removing the powermock and leaving the Do we want to keep the |
21e802f
to
a8bd568
Compare
Whitebox.setInternalState(importOrderOptionMock, "name", "NEW_OPTION_FOR_UT"); | ||
Whitebox.setInternalState(importOrderOptionMock, "ordinal", 5); | ||
Whitebox.setInternalState(mock, "option", importOrderOptionMock); | ||
TestUtil.getClassDeclaredField(ImportOrderCheck.class, "option").set(mock, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IllegalStateException
still occurs if we pass option in as null
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please put a comment above to explain reflection usage, we agreed some time ago that we will do this.
If you additionally remove @Test(expected = IllegalStateException.class)
... it would be awesome ... I thought we already fobid it by some check..... but if you do not want ... we can postpone this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approve.
minor:
Whitebox.setInternalState(importOrderOptionMock, "name", "NEW_OPTION_FOR_UT"); | ||
Whitebox.setInternalState(importOrderOptionMock, "ordinal", 5); | ||
Whitebox.setInternalState(mock, "option", importOrderOptionMock); | ||
TestUtil.getClassDeclaredField(ImportOrderCheck.class, "option").set(mock, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please put a comment above to explain reflection usage, we agreed some time ago that we will do this.
If you additionally remove @Test(expected = IllegalStateException.class)
... it would be awesome ... I thought we already fobid it by some check..... but if you do not want ... we can postpone this.
a8bd568
to
1ebd766
Compare
Done. |
After examining issues at #6421 (comment) ,
I looked into this memory and noticed ImportOrderCheck was using like 8 gigs of memory and 25 minutes to complete pitest. When looking at the report only 1 line said memory error. I changed the code around this mutation and now it completes the 1 check under a minute with no excess memory usage. Test was removed because code changed was directly connected to it.
I don't know if this fixes random issue we have seen in CI. It could be related since probably not all CIs will have huge chunks of memory for usage.
As a side note, this also helps solve #3567 as that test was still waiting further review.