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

pitest: increase mutation coverage for pitest-checkstyle-main profile to 100% #4399

Closed
Nimfadora opened this Issue May 31, 2017 · 5 comments

Comments

Projects
2 participants
@Nimfadora
Contributor

Nimfadora commented May 31, 2017

We created pitest profiles for non-checks code in #4367. Currently, we should increase coverage for pitest-checkstyle-main profile up to 100%.
This issue is a subtask of #3708

Current threshold of pitest-checkstyle-main profile: 93

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jul 10, 2017

Contributor

@romani what should we do with the mutation cases mentioned here

All other cases are connected to different ways of closing IO streams. I can test them with PowerMock, but as it uses reflection it is in a conflict with this Main test. They cannot coexist, one of them fails when I try to fix another.

Contributor

Nimfadora commented Jul 10, 2017

@romani what should we do with the mutation cases mentioned here

All other cases are connected to different ways of closing IO streams. I can test them with PowerMock, but as it uses reflection it is in a conflict with this Main test. They cannot coexist, one of them fails when I try to fix another.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 10, 2017

Member

I can test them with PowerMock, but as it uses reflection it is in a conflict with this Main test. They cannot coexist, one of them fails when I try to fix another.

such test exists only for satisfy coverage , I am to change it at any possible way to avoid conflicts.
I value pitest test more in that case.

Member

romani commented Jul 10, 2017

I can test them with PowerMock, but as it uses reflection it is in a conflict with this Main test. They cannot coexist, one of them fails when I try to fix another.

such test exists only for satisfy coverage , I am to change it at any possible way to avoid conflicts.
I value pitest test more in that case.

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jul 26, 2017

Contributor

such test exists only for satisfy coverage , I am to change it at any possible way to avoid conflicts.
I value pitest test more in that case.

@romani there are two solutions here:

  1. disable calls to CommonUtils and Closables in pitest main profile
  2. remove check of private constructor (decrease cobertura) and use PowerMock to check that Closeables and CommonUtils close streams. (private constructor check and PowerMock could not coexist, as PowerMockRunner somehow makes all private public)

I found work-around with negation issue : we can simply make two ifs inside one another - that kills all mutations.

After we decide what to do, I will quickly increase main coverage to real 100.

Contributor

Nimfadora commented Jul 26, 2017

such test exists only for satisfy coverage , I am to change it at any possible way to avoid conflicts.
I value pitest test more in that case.

@romani there are two solutions here:

  1. disable calls to CommonUtils and Closables in pitest main profile
  2. remove check of private constructor (decrease cobertura) and use PowerMock to check that Closeables and CommonUtils close streams. (private constructor check and PowerMock could not coexist, as PowerMockRunner somehow makes all private public)

I found work-around with negation issue : we can simply make two ifs inside one another - that kills all mutations.

After we decide what to do, I will quickly increase main coverage to real 100.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 26, 2017

Member

disable calls to CommonUtils and Closables in pitest main profile

only as last resort.

remove check of private constructor .... as PowerMockRunner somehow makes all private public

looks like whole problem is due to implementation of assertUtilsClassHasPrivateConstructor https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/internal/TestUtils.java#L50
please make new argument in this method to skip if (!Modifier.isPrivate(constructor.getModifiers())) {
. This argument will be used when Test using PowerMock Runner.
Please try.

we can simply make two ifs inside one another - that kills all mutations.

ok lets try, there will be violations from Tc and pmd and ..... all of them could be suppressed till pitest resolve the issue.

Member

romani commented Jul 26, 2017

disable calls to CommonUtils and Closables in pitest main profile

only as last resort.

remove check of private constructor .... as PowerMockRunner somehow makes all private public

looks like whole problem is due to implementation of assertUtilsClassHasPrivateConstructor https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/internal/TestUtils.java#L50
please make new argument in this method to skip if (!Modifier.isPrivate(constructor.getModifiers())) {
. This argument will be used when Test using PowerMock Runner.
Please try.

we can simply make two ifs inside one another - that kills all mutations.

ok lets try, there will be violations from Tc and pmd and ..... all of them could be suppressed till pitest resolve the issue.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 27, 2017

Member

Final fix is merged

Member

romani commented Jul 27, 2017

Final fix is merged

@romani romani closed this Jul 27, 2017

@romani romani moved this from In Progress to Done in Practice What You Preach Jul 27, 2017

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