Skip to content
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

Filter out synthetic classes during generation of report #668

Merged
merged 1 commit into from
Apr 4, 2018

Conversation

Godin
Copy link
Member

@Godin Godin commented Apr 3, 2018

Fixes #660

@Godin
Copy link
Member Author

Godin commented Apr 3, 2018

@marchof could you please have a look? I can pull down implementation into existing SyntheticFilter, or create a new one, but on the other hand why not skip them entirely?

@Godin Godin requested a review from marchof April 3, 2018 16:05
@Godin Godin added this to the 0.8.2 milestone Apr 3, 2018
@marchof
Copy link
Member

marchof commented Apr 3, 2018

@Godin For now I think this can be hardcoded in Analyzer. In case we need to extend the filtering API to classes for other use cases we can still move it to the new API.

@Godin
Copy link
Member Author

Godin commented Apr 3, 2018

@marchof in this case could you please approve and I will do squash and merge 😉

Copy link
Member

@marchof marchof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank!


import org.jacoco.core.analysis.ICounter;
import org.jacoco.core.test.filter.targets.EnumSwitch;
import org.jacoco.core.test.validation.JavaVersion;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import gives warning.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marchof unused import removed

@Test
public void testCoverageResult() {
if (isJDKCompiler && JAVA_VERSION.isBefore("1.6")) {
assertLine("switch", ICounter.PARTLY_COVERED, 0, 2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we leave a comment why this is the case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marchof added comment

<h3>New Features</h3>
<ul>
<li>Synthetic classes are filtered out during generation of report
(GitHub <a href="https://github.com/jacoco/jacoco/issues/668">#668</a>).</li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is our rule here: Do we refer to the PR or to the issue?

Copy link
Member Author

@Godin Godin Apr 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marchof I prefer to refer to actual changes, so to PR, and was following this principle here and before.

Copy link
Member

@marchof marchof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@marchof marchof merged commit 26a7a02 into master Apr 4, 2018
@marchof marchof deleted the issue-660 branch April 4, 2018 04:22
@Godin Godin moved this from IN PROGRESS to DONE in Filtering Apr 4, 2018
@jacoco jacoco locked as resolved and limited conversation to collaborators Oct 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Filtering
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants