Fix for MethodSorterTest so that coverage can be measured (issue #551) #555
Conversation
Added an extra test case providing an alternative implementation for the getMethodsNullSorter test case. The new implementation passes when coverage analysis using jacoco is enabled, whereas the old one incorrectly fails. The new approach only checks the order of methods that are of interest (i.e., not looking at irrelevant extra methods that the coverage analyzer might generate). Furthermore the new approach avoid the 'sensitive equality' test smell by replacing toString-based equality by actual equality.
Created a separate test case for null sorter applied to methods from the *sub* class. Fixed this test case so that it still works well under (jacoco) coverage. Incomplete - task : Resolve sensitive equality in MethodSorter test.
Incomplete - task : Resolve sensitive equality in MethodSorter test.
Incomplete - task : Issue 551: Resolve sensitive equality in MethodSorter test.
Adjusted test of Default Sorter so that its equality check only deals with actually declared methods, fixing issue 551 for this test case. Incomplete - task : Issue 551: Resolve sensitive equality in MethodSorter test.
Adjusted test of Ascending Sorter so that its equality check only deals with actually declared methods, fixing issue 551 for this test case. Incomplete - task : Issue 551: Resolve sensitive equality in MethodSorter test.
…into junit-coverage
Incomplete - task : Issue 551: Resolve sensitive equality in MethodSorter test.
Incomplete - task : Issue 551: Resolve sensitive equality in MethodSorter test.
Removed the string based equality, and improved naming (in particular calling the expected result 'actual' was confusing). Incomplete - task : Issue 551: Resolve sensitive equality in MethodSorter test.
Since new tests were added to replace all failing test cases, I deleted the old ones that fail with coverage analysis enabled, as well as the helper methods they need. Incomplete - task : Issue 551: Resolve sensitive equality in MethodSorter test.
Incomplete - task : Issue 551: Resolve sensitive equality in MethodSorter test.
The inline method refactoring conducted by eclipse introduced some extra tabs. Incomplete - task : Issue 551: Resolve sensitive equality in MethodSorter test.
Incomplete - task : Issue 551: Resolve sensitive equality in MethodSorter test.
Incomplete - task : Issue 551: Resolve sensitive equality in MethodSorter test.
|
Thanks, looks good to me! However, we'll have to wait until @dsaff is back. |
|
That's kind of strange, when expected values are passed to the function obtaining the actual result. |
|
You're right, it's a bit weird to include the expected result in the function computing the actual result. The better solution may be to consider the filtering as part of the assertion. Makes more sense to me. I can make this change -- either as just a Boolean verification function invoked with an |
|
My assumption was correct, I've just tried and there is additional method "private static final boolean[] $jacocoInit()" with modifiers 0x101a, i.e. synthetic (0x1000) + final (0x10) + static (0x8) + private (0x2). Synthetic methods should be ignored anyway, as they are used in other cases too. |
|
OK, that's very nice. Then I'll follow your suggestion: |
|
@avandeursen, sounds from your last comment like you have more changes coming? |
|
Yes. |
Methods that are synthetic are no longer taken into account when comparing method sorting results. As a result, the tests work fine both with and without test coverage enabled (which may generate synthetic methods). This resolves issue junit-team#551 (and simplifies the earlier fix).
And eliminated unnecessary new String{..}
|
Adjusted according to suggestion by @panchenko. Thanks! Better this way, and ready as far as I'm concerned. |
|
I would suggest filtreing them in MethodSorter.getDeclaredMethods(clazz) actually, why only in test? |
|
That sounds like a feature request: JUnit should not execute test cases encoded in synthesized methods. I can, however, imagine legitimate uses of synthesized test cases: For example when annotations are used to generate additional test cases, which then could (should?) have the synthesized bit enabled (test generation in the style of project lombok). My suggestion would be to merge this pull request first so that JUnit can be tested with coverage enabled, and optionally issue a separate feature request for filtering out synthesized methods. |
Fix for MethodSorterTest so that coverage can be measured (issue #551)
|
Thanks for the fix! |
4 participants
The issue (#551):
With Jacoco (formerly eclemma) code coverage enabled in Eclipse, the
org.junit.internal.MethodSorterTestfailed ongetMethodsNullSortertestDefaultSortertestNameAscThe cause was that via reflection methods were collected from static classes. These included temporary methods generated by the coverage tool, which were not expected by the test causing the test to fail.
The fix
To fix this, I filter methods before comparison, to just the methods actually declared. This is done by the new
getDeclaredFilteredMethodshelper method. As a result, all tests pass both with and without coverage analysis enabled.I also applied some small refactorings. I extracted separate test cases for method sorting on sub and superclasses (which were included in the null sorter test case). Furthermore, I based all equality checks directly on lists or arrays, instead of on the
toStringrepresentation of objects.