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

Pull #5351: fixed RequireThisCheck and catch variable handling #5351

Merged
merged 2 commits into from Dec 14, 2017

Conversation

Projects
None yet
3 participants
@rnveach
Member

rnveach commented Dec 13, 2017

Identified at #5307 (comment)

Catch variables are seen in the scope of the try block instead of the catch block.

Easy printout of frame tree with this changes: rnveach@aad8b03#diff-6b1f89f1bde0470d4bd42580d1aa8bfb
Regression: http://rveach.no-ip.org/checkstyle/regression/reports/143/
http://rveach.no-ip.org/checkstyle/regression/reports/143/spring-framework/index.html#A1

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 13, 2017

Member

From travis

forbiddenapis:2.4.1:testCheck
....
[ERROR] Forbidden method invocation: java.lang.Throwable#printStackTrace() [Eclipse auto-generated stubs; exceptions should be correctly bubbled up and handled accordingly]

@romani Why are we checking our input files? Should I just exclude the file as it is a replica of a regression case?

Member

rnveach commented Dec 13, 2017

From travis

forbiddenapis:2.4.1:testCheck
....
[ERROR] Forbidden method invocation: java.lang.Throwable#printStackTrace() [Eclipse auto-generated stubs; exceptions should be correctly bubbled up and handled accordingly]

@romani Why are we checking our input files? Should I just exclude the file as it is a replica of a regression case?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 13, 2017

Member

Why are we checking our input files?

This plugin should not validate any test resource files. Please review plugin properties to exclude or point plugin to only required classes.

Member

romani commented Dec 13, 2017

Why are we checking our input files?

This plugin should not validate any test resource files. Please review plugin properties to exclude or point plugin to only required classes.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 13, 2017

Member

<td class="numLineCover">&nbsp;1375</td>

Another oddity, code coverage is missing on class definition line, not any instructions.
What am I missing?

According to decompiling the clas file, there is an extra method generated getFrameNameIdent, so I am assuming that is it.

Uncovered items:target/pit-reports/201712130307/com.puppycrawl.tools.checkstyle.filters/SeverityMatchFilter.java.html:<td class='uncovered'> target/pit-reports/201712130307/com.puppycrawl.tools.checkstyle.filters/SeverityMatchFilter.java.html:<td class='uncovered'><pre><span class=''> }</span></pre></td></tr>
Survived/Uncovered items found in reports, build will be failed

Pitest is also joining in, though I'm not sure why.

Member

rnveach commented Dec 13, 2017

<td class="numLineCover">&nbsp;1375</td>

Another oddity, code coverage is missing on class definition line, not any instructions.
What am I missing?

According to decompiling the clas file, there is an extra method generated getFrameNameIdent, so I am assuming that is it.

Uncovered items:target/pit-reports/201712130307/com.puppycrawl.tools.checkstyle.filters/SeverityMatchFilter.java.html:<td class='uncovered'> target/pit-reports/201712130307/com.puppycrawl.tools.checkstyle.filters/SeverityMatchFilter.java.html:<td class='uncovered'><pre><span class=''> }</span></pre></td></tr>
Survived/Uncovered items found in reports, build will be failed

Pitest is also joining in, though I'm not sure why.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 13, 2017

Member

This plugin should not validate any test resource files.

See #3402 on when it was activated on input files.
It is related to Issue #2541 .

Member

rnveach commented Dec 13, 2017

This plugin should not validate any test resource files.

See #3402 on when it was activated on input files.
It is related to Issue #2541 .

rnveach added a commit to rnveach/checkstyle that referenced this pull request Dec 13, 2017

rnveach added a commit to rnveach/checkstyle that referenced this pull request Dec 13, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 13, 2017

Member

Code coverage should be fixed.
Shippable failure will be fixed once #5354 is merged, as it is un-related to this issue.

Member

rnveach commented Dec 13, 2017

Code coverage should be fixed.
Shippable failure will be fixed once #5354 is merged, as it is un-related to this issue.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Dec 14, 2017

Codecov Report

Merging #5351 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #5351   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         296     296           
  Lines       16189   16199   +10     
  Branches     3698    3699    +1     
======================================
+ Hits        16189   16199   +10
Impacted Files Coverage Δ
...ols/checkstyle/checks/coding/RequireThisCheck.java 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 034e24e...0a23cd3. Read the comment docs.

codecov-io commented Dec 14, 2017

Codecov Report

Merging #5351 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #5351   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         296     296           
  Lines       16189   16199   +10     
  Branches     3698    3699    +1     
======================================
+ Hits        16189   16199   +10
Impacted Files Coverage Δ
...ols/checkstyle/checks/coding/RequireThisCheck.java 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 034e24e...0a23cd3. Read the comment docs.

@rnveach rnveach requested a review from romani Dec 14, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 14, 2017

Member

Rebased.

Member

rnveach commented Dec 14, 2017

Rebased.

@romani romani merged commit c5e71f7 into checkstyle:master Dec 14, 2017

6 of 7 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Codacy/PR Quality Review Good work! A positive pull request.
Details
IDEA Inspections Pull Request (Checkstyle) TeamCity build finished
Details
Shippable Run 5766 status is SUCCESS.
Details
continuous-integration/Distelli
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
wercker/build Wercker pipeline passed
Details

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

@rnveach rnveach deleted the rnveach:pr_5307_1 branch Dec 14, 2017

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

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