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 #5113: enabled eclipse compiler to flag unused exceptions #5113

Merged
merged 1 commit into from Sep 28, 2017

Conversation

Projects
None yet
3 participants
@rnveach
Member

rnveach commented Sep 15, 2017

This change configures eclipse compiler check to flag unused exceptions in tests.
We should only throw exceptions as needed and this check will make sure no excess exceptions are being marked.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 15, 2017

Codecov Report

Merging #5113 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #5113   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         296     296           
  Lines       16098   16098           
  Branches     3642    3642           
======================================
  Hits        16098   16098

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 32f0251...33f9f1a. Read the comment docs.

codecov-io commented Sep 15, 2017

Codecov Report

Merging #5113 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #5113   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         296     296           
  Lines       16098   16098           
  Branches     3642    3642           
======================================
  Hits        16098   16098

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 32f0251...33f9f1a. Read the comment docs.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Sep 15, 2017

Member

It doesn't seem to flag anything from the test directories as I am getting violations in my eclipse even on other things besides unused exceptions.
I will need to look into the reason. I am thinking the classpath isn't receiving the tests.

Member

rnveach commented Sep 15, 2017

It doesn't seem to flag anything from the test directories as I am getting violations in my eclipse even on other things besides unused exceptions.
I will need to look into the reason. I am thinking the classpath isn't receiving the tests.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Sep 15, 2017

Member

Along with the classpath being changed, the shell script has to be modified to include the test folder.

When doing so, it results in ~24 errors in master which I am also seeing locally inside Eclipse.
Most are The allocated object is never used, The declared exception CheckstyleException is not actually thrown, constructor/methods never being used, etc...

It looks like if we enable this in test, we would have to suppress these areas as they are just ways of testing.

Member

rnveach commented Sep 15, 2017

Along with the classpath being changed, the shell script has to be modified to include the test folder.

When doing so, it results in ~24 errors in master which I am also seeing locally inside Eclipse.
Most are The allocated object is never used, The declared exception CheckstyleException is not actually thrown, constructor/methods never being used, etc...

It looks like if we enable this in test, we would have to suppress these areas as they are just ways of testing.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Sep 25, 2017

Member

This PR is dependent on #5143 first.

Member

rnveach commented Sep 25, 2017

This PR is dependent on #5143 first.

rnveach added a commit to rnveach/checkstyle that referenced this pull request Sep 25, 2017

@rnveach rnveach changed the title from Pull #XXX: flag unused exceptions in tests to Pull #5113: enabled eclipse compiler to flag unused exceptions Sep 25, 2017

rnveach added a commit to rnveach/checkstyle that referenced this pull request Sep 25, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Sep 25, 2017

Member

@romani This PR is ready for review.

Member

rnveach commented Sep 25, 2017

@romani This PR is ready for review.

rnveach added a commit to rnveach/checkstyle that referenced this pull request Sep 25, 2017

@rnveach rnveach requested a review from romani Sep 27, 2017

@romani

I do not see reason of two commits, please squash.

please put description comment to each "disabled"

rnveach added a commit to rnveach/checkstyle that referenced this pull request Sep 28, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach
Member

rnveach commented Sep 28, 2017

@romani done.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 28, 2017

Member

Travis failure is valid:

1. ERROR in /home/travis/build/checkstyle/checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/utils/FilterUtilsTest.java (at line 61)
	public void testNonExistingFile() throws Exception {
	                                         ^^^^^^^^^
The declared exception Exception is not actually thrown by the method testNonExistingFile() from type FilterUtilsTest
Member

romani commented Sep 28, 2017

Travis failure is valid:

1. ERROR in /home/travis/build/checkstyle/checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/utils/FilterUtilsTest.java (at line 61)
	public void testNonExistingFile() throws Exception {
	                                         ^^^^^^^^^
The declared exception Exception is not actually thrown by the method testNonExistingFile() from type FilterUtilsTest
@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Sep 28, 2017

Member

@romani Fixed.

Member

rnveach commented Sep 28, 2017

@romani Fixed.

@romani romani merged commit 1d760d4 into checkstyle:master Sep 28, 2017

6 of 7 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
IDEA Inspections Pull Request (Checkstyle) TeamCity build finished
Details
Shippable Run 5256 status is SUCCESS.
Details
codacy/pr Good work! A positive pull request.
Details
continuous-integration/Distelli
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
wercker/build Wercker pipeline passed
Details
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 28, 2017

Member

Travis failures were unrelated

Member

romani commented Sep 28, 2017

Travis failures were unrelated

@romani romani added this to the 8.3 milestone Sep 28, 2017

@romani romani added the approved label Sep 28, 2017

@rnveach rnveach deleted the rnveach:unused_exceptions branch Sep 28, 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