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

minimize pitest-checkstyle-utils profile execution #4605

Open
romani opened this Issue Jul 4, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@romani
Member

romani commented Jul 4, 2017

problem was detected at #4596 (review)

pitest-checkstyle-utils

has too much tests in it:

  <targetTests>
    <param>com.puppycrawl.tools.checkstyle.utils.*</param>
    <param>com.puppycrawl.tools.checkstyle.AstTreeStringPrinterTest</param>
    <param>com.puppycrawl.tools.checkstyle.ConfigurationLoaderTest</param>
    <param>com.puppycrawl.tools.checkstyle.DetailNodeTreeStringPrinterTest</param>
    <param>com.puppycrawl.tools.checkstyle.PackageObjectFactoryTest</param>
    <param>com.puppycrawl.tools.checkstyle.checks.FinalParametersCheckTest</param>
    <param>com.puppycrawl.tools.checkstyle.checks.TranslationCheckTest</param>
    <param>com.puppycrawl.tools.checkstyle.checks.blocks.RightCurlyCheckTest</param>
    <param>com.puppycrawl.tools.checkstyle.checks.coding.MultipleVariableDeclarationsCheckTest</param>
    <param>com.puppycrawl.tools.checkstyle.checks.design.FinalClassCheckTest</param>
    <param>com.puppycrawl.tools.checkstyle.checks.javadoc.*</param>
    <param>com.puppycrawl.tools.checkstyle.checks.regexp.RegexpOnFilenameCheckTest</param>
    <param>com.puppycrawl.tools.checkstyle.checks.whitespace.*</param>
    <param>com.puppycrawl.tools.checkstyle.filters.SuppressionCommentFilter</param>
    <param>com.puppycrawl.tools.checkstyle.filters.SuppressWithNearbyCommentFilterTest</param>
    <param>com.puppycrawl.tools.checkstyle.internal.AllChecksTest</param>
  </targetTests>

we should avoid usage of all Check test - com.puppycrawl.tools.checkstyle.checks
if we need some test methods, we should copy them to Utils test.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 10, 2017

Member

http://rveach.no-ip.org/checkstyle/regression/pitest-reports/12/

pitest with all excess tests removed except for it's own package.
Besides mutation at 88%, code coverage is at 91%. So both need to be fixed.

Member

rnveach commented Dec 10, 2017

http://rveach.no-ip.org/checkstyle/regression/pitest-reports/12/

pitest with all excess tests removed except for it's own package.
Besides mutation at 88%, code coverage is at 91%. So both need to be fixed.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 13, 2017

Member

I do think we need to close this issue, as it does not make a lot of sense to focus on it. Performance is ok.
I would only address wildcard targets:
"com.puppycrawl.tools.checkstyle.checks.coding."
"com.puppycrawl.tools.checkstyle.checks.javadoc.
"
to use exact Checks instead.

@rnveach , what is your ideas on this ?

Member

romani commented Dec 13, 2017

I do think we need to close this issue, as it does not make a lot of sense to focus on it. Performance is ok.
I would only address wildcard targets:
"com.puppycrawl.tools.checkstyle.checks.coding."
"com.puppycrawl.tools.checkstyle.checks.javadoc.
"
to use exact Checks instead.

@rnveach , what is your ideas on this ?

@romani romani added this to the 8.6 milestone Dec 13, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 13, 2017

Member

like I already stated in PR, I would like to see utils have their own separate tests and coverage and not have to rely on external checks.

PackageObjectFactoryTest should be able to be removed as it is only for 1 method in ModuleReflectionUtils.

wildcard targets

this makes sense too.

Member

rnveach commented Dec 13, 2017

like I already stated in PR, I would like to see utils have their own separate tests and coverage and not have to rely on external checks.

PackageObjectFactoryTest should be able to be removed as it is only for 1 method in ModuleReflectionUtils.

wildcard targets

this makes sense too.

rnveach added a commit to rnveach/checkstyle that referenced this issue Dec 14, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Dec 14, 2017

romani added a commit that referenced this issue Dec 14, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

romani added a commit that referenced this issue Feb 26, 2018

romani added a commit that referenced this issue Feb 26, 2018

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 31, 2018

Member

https://github.com/checkstyle/checkstyle/blob/master/pom.xml#L2441

current state is:

<targetTests>
    <param>com.puppycrawl.tools.checkstyle.utils.*</param>
    <!-- 12% mutation in CommonUtils, 3% coverage in CommonUtils, 2% coverage in JavadocUtils -->
    <param>com.puppycrawl.tools.checkstyle.AstTreeStringPrinterTest</param>
    <!-- 2% mutation in CommonUtils -->
    <param>com.puppycrawl.tools.checkstyle.DetailNodeTreeStringPrinterTest</param>
    <!-- ModuleReflectionUtils -->
    <param>com.puppycrawl.tools.checkstyle.PackageObjectFactoryTest</param>
    <!-- 3% coverage in BlockCommentPosition, 11% mutation in JavadocUtils, 10% coverage in JavadocUtils, ScopeUtils -->
    <param>com.puppycrawl.tools.checkstyle.checks.javadoc.AbstractJavadocCheckTest</param>
    <param>com.puppycrawl.tools.checkstyle.checks.javadoc.SingleLineJavadocCheckTest</param>
    <param>com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocVariableCheckTest</param>
</targetTests>

duration of build is 3 min, https://app.shippable.com/github/checkstyle/checkstyle/runs/6609/24/console.

@rnveach , do we still need to finish optimization of it, is it reasonable ?

Member

romani commented Mar 31, 2018

https://github.com/checkstyle/checkstyle/blob/master/pom.xml#L2441

current state is:

<targetTests>
    <param>com.puppycrawl.tools.checkstyle.utils.*</param>
    <!-- 12% mutation in CommonUtils, 3% coverage in CommonUtils, 2% coverage in JavadocUtils -->
    <param>com.puppycrawl.tools.checkstyle.AstTreeStringPrinterTest</param>
    <!-- 2% mutation in CommonUtils -->
    <param>com.puppycrawl.tools.checkstyle.DetailNodeTreeStringPrinterTest</param>
    <!-- ModuleReflectionUtils -->
    <param>com.puppycrawl.tools.checkstyle.PackageObjectFactoryTest</param>
    <!-- 3% coverage in BlockCommentPosition, 11% mutation in JavadocUtils, 10% coverage in JavadocUtils, ScopeUtils -->
    <param>com.puppycrawl.tools.checkstyle.checks.javadoc.AbstractJavadocCheckTest</param>
    <param>com.puppycrawl.tools.checkstyle.checks.javadoc.SingleLineJavadocCheckTest</param>
    <param>com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocVariableCheckTest</param>
</targetTests>

duration of build is 3 min, https://app.shippable.com/github/checkstyle/checkstyle/runs/6609/24/console.

@rnveach , do we still need to finish optimization of it, is it reasonable ?

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