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 #6439: move powermock tests away from the normal tests #6445

Merged
merged 2 commits into from Feb 23, 2019

Conversation

rnveach
Copy link
Member

@rnveach rnveach commented Feb 20, 2019

Issue #6439

I am expecting pitest to fail until I add new suppressions.

<!-- Conflicts with normal tests and pitest.
See examples in https://github.com/checkstyle/checkstyle/issues/6439 -->
<allow class="org.powermock.reflect.Whitebox" />
<allow class="org.mockito.internal.util.Checks" />
Copy link
Member Author

Choose a reason for hiding this comment

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

These are ok since they use reflection and not byte code hacking.

@@ -57,7 +57,7 @@
* configuration has changed.
*
*/
final class PropertyCacheFile {
public final class PropertyCacheFile {
Copy link
Member Author

Choose a reason for hiding this comment

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

Can't tag the class for powermock since it is package private.

@@ -40,7 +40,7 @@
/**
* Responsible for loading the contents of an import control configuration file.
*/
final class ImportControlLoader extends XmlLoader {
public final class ImportControlLoader extends XmlLoader {
Copy link
Member Author

Choose a reason for hiding this comment

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

Can't tag the class for powermock since it is package private.

/**
* Non meaningful javadoc just to contain "noinspection" tag.
* Till https://youtrack.jetbrains.com/issue/IDEA-187210
* @noinspection JUnitTestCaseWithNoTests
Copy link
Member Author

Choose a reason for hiding this comment

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

With powermock gone, the false positives went away too.

@rnveach rnveach requested a review from romani February 21, 2019 02:05
@romani
Copy link
Member

romani commented Feb 21, 2019

We got bunch of items to our ignore list, but what we get as benefits ?

@rnveach
Copy link
Member Author

rnveach commented Feb 21, 2019

@romani The biggest benefit is the pitests run faster. We are talking 36.3 minutes versus 84.9 minutes, so that saves us 48 minutes.
I also mentioned in the issue the other benefits where powermock kept screwing us.
Some of the pitest.sh additions are simply because we overrely on powermock. PackageNamesLoader is the best example. It is hard coded to only bring in packages from a specific file. We had to use powermock because we couldn't modify the file in main. This class can easily be fixed to just need reflection to "pretend" we got the values from the file.

As always, I will continue to work on killing the list of mutants.

Old Times:

Total time: 00:01
Total time: 00:40
Total time: 03:43 min
Total time: 01:06 min
Total time: 02:07 min
Total time: 05:47 min
Total time: 00:37
Total time: 02:02 min
Total time: 01:28 min
Total time: 04:04 min
Total time: 03:15 min
Total time: 00:27
Total time: 01:06 min
Total time: 00:30
Total time: 00:25
Total time: 00:18
Total time: 00:55
Total time: 10:48 min
Total time: 01:38 min
Total time: 07:23 min
Total time: 01:13 min
Total time: 19:24 min
Total time: 03:57 min
Total time: 03:07 min
Total time: 01:46 min
Total time: 02:23 min
Total time: 01:03 min
Total time: 03:44 min

New Times:

Total time: 00:01
Total time: 00:46
Total time: 01:22 min
Total time: 01:06 min
Total time: 02:07 min
Total time: 05:51 min
Total time: 00:38
Total time: 01:04 min
Total time: 00:50
Total time: 04:05 min
Total time: 02:52 min
Total time: 00:30
Total time: 01:06 min
Total time: 00:31
Total time: 00:24
Total time: 00:19
Total time: 00:55
Total time: 00:20
Total time: 00:09
Total time: 01:38 min
Total time: 00:17
Total time: 00:47
Total time: 01:01 min
Total time: 01:45 min
Total time: 00:40
Total time: 01:20 min
Total time: 00:17
Total time: 03:41 min

The longest running job is 5 minutes instead of 19 minutes. Second longest job is 4 minutes instead of 10 minutes.

@rnveach
Copy link
Member Author

rnveach commented Feb 22, 2019

@romani I will work on killing some bigger gaps in this PR, starting with PackageNamesLoader. It is basically like swapping powermock tests for normal tests.

Edit: All powermock tests for PackageNamesLoader were changed to normal tests. No reflection was used.
Edit: 1 was swapped for HeaderCheck, the other can't be swapped because StringReader is pretty much exception safe but can only throw an exception if the Reader is closed more then once, which is impossible to do without powermock.

@romani
Copy link
Member

romani commented Feb 22, 2019

Ok to merge

@rnveach
Copy link
Member Author

rnveach commented Feb 22, 2019

@romani CI is green. Do you want to merge as 3 commits or want me to squash them?

@rnveach rnveach force-pushed the issue_6439 branch 2 times, most recently from 26c25c3 to b61e644 Compare February 23, 2019 01:49
@romani
Copy link
Member

romani commented Feb 23, 2019

Whatever.

@rnveach
Copy link
Member Author

rnveach commented Feb 23, 2019

I squashed the commits and will merge when CI is done.

@rnveach rnveach merged commit 4b8e508 into checkstyle:master Feb 23, 2019
@rnveach rnveach deleted the issue_6439 branch February 23, 2019 11:47
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

2 participants