Allowed more tests to be considered killing a mutant when using historic data (fixes #763) #765
Conversation
fixes hcoles#763 Incremental analysis honors all killing tests now instead of limiting the scope of analysis to the first killing test. This is often identical if there is no matrix mutation in place but TestNG plugin calculates test units in terms of test classes thus executes all test methods of a test class and records historic data with method level granularity and class level granularity. We need to take all killing tests into account to match history data and executable test units in the analyzer. This will also help if different test classes cover a certain production class and one of those test classes is deleted or renamed. Improved logging to help spotting analyzer misbehaviour.
Changing the configuration of the Logger is a global setting per JVM. When running tests with surefire the JVM will be reused among tests. Permanently changing the logging configuration might interfere with other tests.
| for (Handler handler : logger.getHandlers()) { | ||
| logger.removeHandler(handler); | ||
| } | ||
| logger.addHandler(logCatcher); |
There was a problem hiding this comment.
@StefanPenndorf Are we introducing a memory leak into the test suite here? If nothing removes the LogCatcher when the last runs then presumably it continues to accumulate log entries until the JVM exits?
There was a problem hiding this comment.
Yes, my first commit had this issue: It introduced a memory leak as well as changing the behaviour for subsequent tests. Thus I decided not to disable default handlers by removing them and to remove the log catcher in a tear down method (annotated with @After)
I guess you found the fix because you merged my change in anyway.
There was a problem hiding this comment.
Saw your change come in so merged it - hopefully not prematurely?
Tracked down a leak exactly like this one in a commercial test suite years ago, so knew to look for it when testing logging.
There was a problem hiding this comment.
Everything well, pull request was complete with two commits. Would have left a note if I knew there was something wrong/missing.
fixes #763
Incremental analysis honors all killing tests now instead of limiting the scope of analysis to the first killing test.
This is often identical if there is no matrix mutation in place but TestNG plugin calculates test units in terms of test classes thus executes all test methods of a test class and records historic data
with method level granularity and class level granularity. We need to take all killing tests into account to match history data and executable test units in the analyzer. This will also help if different test classes cover a certain production class and one of those test classes is deleted or renamed.
Improved logging to help spotting analyzer misbehaviour. Although pitest is already verbose I added another info-level statement.
I've also refactored the test and added more test cases to prove the correctness of my changes.