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 from a report private empty constructors that do not have arguments #529

Merged
merged 1 commit into from May 9, 2017

Conversation

Projects
4 participants
@Godin
Member

Godin commented May 6, 2017

After reflecting for some time, I believe that we can unconditionally safely ignore private empty constructors that do not have arguments. Even despite the fact that they relate to a source code and not generated by compiler. Note that this will completely exclude from report unused classes that have only such constructor, but anyway revealing of those is not a job of code coverage tool.

This will also improve report for JaCoCo itself.

@Godin Godin added this to the 0.8.0 milestone May 6, 2017

@Godin Godin self-assigned this May 6, 2017

@Godin Godin added this to IN PROGRESS in Filtering May 6, 2017

@Godin Godin added this to IN PROGRESS in Current work items May 6, 2017

@Godin Godin requested a review from marchof May 6, 2017

@marchof

@Godin This filter is definitely a quick win! I have two minor comments to improve the matcher API.

if (m != null && superClassName.equals(m.owner)
&& "<init>".equals(m.name) && ("()V").equals(m.desc)) {
nextIs(Opcodes.RETURN);
return cursor != null;

This comment has been minimized.

@marchof

marchof May 7, 2017

Member

If nextIs() would have an boolean return value, we could write this as:

return nextIs(Opcodes.Return)

Could also be used in other existing filters.

@marchof

marchof May 7, 2017

Member

If nextIs() would have an boolean return value, we could write this as:

return nextIs(Opcodes.Return)

Could also be used in other existing filters.

This comment has been minimized.

@Godin

Godin May 7, 2017

Member

@marchof then there will be temptation to use it with && everywhere, but that is exactly what we were trying to avoid.

@Godin

Godin May 7, 2017

Member

@marchof then there will be temptation to use it with && everywhere, but that is exactly what we were trying to avoid.

This comment has been minimized.

@marchof

marchof May 8, 2017

Member

@Godin It's internal API, I would try to keep the filter as readable as possible.

@marchof

marchof May 8, 2017

Member

@Godin It's internal API, I would try to keep the filter as readable as possible.

This comment has been minimized.

@Godin

Godin May 8, 2017

Member

@marchof Then in case of preceding if statement containing return of a boolean, IDE start to suggest to combine them, making code unreadable. Also even if I don't see this in IntelliJ, static analyzers might warn about unused return value in other places.

@Godin

Godin May 8, 2017

Member

@marchof Then in case of preceding if statement containing return of a boolean, IDE start to suggest to combine them, making code unreadable. Also even if I don't see this in IntelliJ, static analyzers might warn about unused return value in other places.

Show outdated Hide outdated ...co/core/internal/analysis/filter/PrivateEmptyNoArgConstructorFilter.java Outdated
@marchof

marchof approved these changes May 9, 2017

@Godin From my point of view this can be merged. Please do so if you don't have any open issues.

@Godin Godin merged commit c63563d into master May 9, 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

@Godin Godin deleted the issue-529 branch May 9, 2017

@Godin Godin removed this from IN PROGRESS in Current work items May 9, 2017

@Godin Godin moved this from IN PROGRESS to DONE in Filtering May 9, 2017

@MonsieurBon

This comment has been minimized.

Show comment
Hide comment
@MonsieurBon

MonsieurBon Nov 1, 2017

Do you have an estimate about when this could be released?

MonsieurBon commented Nov 1, 2017

Do you have an estimate about when this could be released?

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Nov 1, 2017

Member

@MonsieurBon Hopefully this year.

You can use our snapshot builds in the meantime and help us testing!

Member

marchof commented Nov 1, 2017

@MonsieurBon Hopefully this year.

You can use our snapshot builds in the meantime and help us testing!

@saadlu

This comment has been minimized.

Show comment
Hide comment
@saadlu

saadlu Nov 6, 2017

@marchof

Snapshot build is fine with me, but I can't seem to test this.
This is what I have done

  1. Checkout the latest master
    As of now, this is the master HEAD (your commit I assume :))

    commit 8f0460960967e15e46c9f2bf766c0bfe9e18e6b8
    Author: Marc R. Hoffmann <hoffmann@mountainminds.com>
    Date:   Thu Nov 2 22:07:24 2017 +0100
    
    Remove obsolete phase binding (#617)
    
    Since version 0.6.2 the "report" goal is bound to phase "verify".
    
  2. built locally (with mvn clean install)

  3. Using a gradle project to test out the local built jacoco

    apply plugin: 'jacoco'
    jacoco {
       toolVersion = "0.7.10-SNAPSHOT"
    }
    

    testing code:

    public class Empty {
    
        private Empty(){}
    
        public static String say(){
            return "say";
        }
     }
    
    

    with

    public class EmptyTest {
       @Test
       public void say() {
         assertThat(Empty.say(), is("say"));
       }
    
    }
    
  4. Using sonar to generate me the report

  5. Sonar says only 50% covered. Complaining that the private constructor is not convered

what am I doing wrong?

saadlu commented Nov 6, 2017

@marchof

Snapshot build is fine with me, but I can't seem to test this.
This is what I have done

  1. Checkout the latest master
    As of now, this is the master HEAD (your commit I assume :))

    commit 8f0460960967e15e46c9f2bf766c0bfe9e18e6b8
    Author: Marc R. Hoffmann <hoffmann@mountainminds.com>
    Date:   Thu Nov 2 22:07:24 2017 +0100
    
    Remove obsolete phase binding (#617)
    
    Since version 0.6.2 the "report" goal is bound to phase "verify".
    
  2. built locally (with mvn clean install)

  3. Using a gradle project to test out the local built jacoco

    apply plugin: 'jacoco'
    jacoco {
       toolVersion = "0.7.10-SNAPSHOT"
    }
    

    testing code:

    public class Empty {
    
        private Empty(){}
    
        public static String say(){
            return "say";
        }
     }
    
    

    with

    public class EmptyTest {
       @Test
       public void say() {
         assertThat(Empty.say(), is("say"));
       }
    
    }
    
  4. Using sonar to generate me the report

  5. Sonar says only 50% covered. Complaining that the private constructor is not convered

what am I doing wrong?

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin
Member

Godin commented Nov 6, 2017

@saadlu see #513 (comment)

@jacoco jacoco locked as resolved and limited conversation to collaborators Jan 11, 2018

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