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

Exclude enum methods "values" and "valueOf" from reports #491

Merged
merged 1 commit into from Feb 27, 2017

Conversation

Projects
2 participants
@Godin
Member

Godin commented Feb 25, 2017

I propose to do a baby step in story of filtering by excluding enum methods values and valueOf from reports.

@Godin Godin self-assigned this Feb 26, 2017

@Godin Godin added this to the 0.7.10 milestone Feb 26, 2017

@Godin Godin requested a review from marchof Feb 26, 2017

@marchof

@Godin Thx! I added some inline comments.

Show outdated Hide outdated org.jacoco.core/src/org/jacoco/core/internal/analysis/ClassAnalyzer.java Outdated
@@ -87,4 +87,24 @@ public void testMethodFilter_Lambda() {
assertEquals(1, coverage.getMethods().size());
}
@Test
public void testMethodFilter_EnumValues() {

This comment has been minimized.

@marchof

marchof Feb 27, 2017

Member

@Godin What about a negative test for different signature?

@marchof

marchof Feb 27, 2017

Member

@Godin What about a negative test for different signature?

This comment has been minimized.

@Godin

Godin Feb 27, 2017

Member

@marchof added

@Godin

Godin Feb 27, 2017

Member

@marchof added

Show outdated Hide outdated ...o.core.test/src/org/jacoco/core/internal/analysis/ClassAnalyzerTest.java Outdated

@Godin Godin requested a review from marchof Feb 27, 2017

fixed

@marchof

@Godin I feel bad for beeing so picky today :-(

final String desc) {
if ("java/lang/Enum".equals(superClassName)) {
// filter out methods that compiler creates for enums
if ("values".equals(name)

This comment has been minimized.

@marchof

marchof Feb 27, 2017

Member

We mix two different comparisons styles here: "expected".equals(actual) vs. actual.equals("expected"). I would prefer to use the first one only as it is NPE safe.

@marchof

marchof Feb 27, 2017

Member

We mix two different comparisons styles here: "expected".equals(actual) vs. actual.equals("expected"). I would prefer to use the first one only as it is NPE safe.

This comment has been minimized.

@Godin

Godin Feb 27, 2017

Member

Umm, below there is anyway name.startsWith(...) that isn't NPE safe. Also according to my knowledge of ASM API - desc can't be null. For the NPE safety IMO far better to ensure that parameters of methods are not null by default and nullability should be stated explicitly.

But ok, I'll swap arguments, hoping that you won't say that concatenation on left hand side looks awkward 😆

@Godin

Godin Feb 27, 2017

Member

Umm, below there is anyway name.startsWith(...) that isn't NPE safe. Also according to my knowledge of ASM API - desc can't be null. For the NPE safety IMO far better to ensure that parameters of methods are not null by default and nullability should be stated explicitly.

But ok, I'll swap arguments, hoping that you won't say that concatenation on left hand side looks awkward 😆

Show outdated Hide outdated ...o.core.test/src/org/jacoco/core/internal/analysis/ClassAnalyzerTest.java Outdated

fixed

@Godin Godin requested a review from marchof Feb 27, 2017

@marchof marchof merged commit 296f992 into master Feb 27, 2017

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@marchof marchof deleted the issue-491 branch Feb 27, 2017

@jacoco jacoco locked and limited conversation to collaborators Feb 27, 2017

@Godin Godin added this to DONE in Filtering Mar 11, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.