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

minor: solved pitest issues with ImportOrderCheck #6424

Merged
merged 1 commit into from Feb 15, 2019
Merged

minor: solved pitest issues with ImportOrderCheck #6424

merged 1 commit into from Feb 15, 2019

Conversation

rnveach
Copy link
Member

@rnveach rnveach commented Feb 14, 2019 •
edited

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.

@rnveach
Copy link
Member Author

rnveach commented Feb 14, 2019

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.

@rnveach rnveach requested a review from romani February 14, 2019 03:52
rnveach
rnveach commented Feb 14, 2019
View reviewed changes
.ci/pitest.sh Outdated Show resolved Hide resolved
@rnveach
Copy link
Member Author

rnveach commented Feb 14, 2019

from logs Total time: 22:28 min, that is not good for sure.

Time dropped now by 20 minutes.

[INFO] Total time: 02:26 min

@rnveach
Copy link
Member Author

rnveach commented Feb 14, 2019 •
edited

I tried testing something out by just removing the powermock and leaving the IllegalStateException in the code and it also reduced the testing time. It seems somehow powermock is responsible for the long time and excess memory. I'm not sure if this is an issue in other tests or somehow this check had something unique about it. I also tried leaving powermock in but removing the powermock test, it still took 27 minutes to finish.

Do we want to keep the IllegalStateException but rewrite the test?

@rnveach rnveach mentioned this pull request Feb 14, 2019
Open
rnveach added a commit to rnveach/checkstyle that referenced this pull request Feb 15, 2019
@rnveach
Pull checkstyle#6424: solved pitest issues with ImportOrderCheck
a8bd568
rnveach
rnveach commented Feb 15, 2019
View reviewed changes
src/test/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportOrderCheckTest.java
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);
Copy link
Member Author

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.

Copy link
Member

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.

romani
romani approved these changes Feb 15, 2019
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.

approve.

minor:

src/test/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportOrderCheckTest.java
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);
Copy link
Member

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.

@romani romani assigned rnveach and unassigned romani Feb 15, 2019
@rnveach
Copy link
Member Author

rnveach commented Feb 15, 2019

I thought we already fobid it by some check..... but if you do not want ... we can postpone this.

@romani See #3567
You wanted to review this item as you didn't like how I removed it before. This is why it still remains.

@rnveach
Copy link
Member Author

rnveach commented Feb 15, 2019

please put a comment above to explain reflection usage

Done.

@rnveach rnveach assigned romani and unassigned rnveach Feb 15, 2019
@rnveach rnveach merged commit d5e6444 into checkstyle:master Feb 15, 2019
@rnveach rnveach deleted the pitest_importorder_issue branch February 15, 2019 03:53
@rnveach rnveach added this to the 8.18 milestone Feb 15, 2019
@rnveach rnveach mentioned this pull request Feb 19, 2019
Closed
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

Assignees

@romani romani

Labels
miscellaneous
Projects
None yet
Milestone
8.18
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@rnveach @romani