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

Checkstyle tests should be named after the class they test and extra validations/helpers should be in a special package #5104

Closed
rnveach opened this Issue Sep 12, 2017 · 9 comments

Comments

Projects
None yet
3 participants
@rnveach
Member

rnveach commented Sep 12, 2017

Identified at checkstyle/regression-tool#85 (comment) ,

To be able to pull in properties from tests to be used in regression on a module, we need a clear connection between the test and the production code. This is usually defined by naming the test class after the production class and having them in the same package.

Some tests break this rule and we should rename/move them so they are in alignment and there is a clear connection between the 2 classes.
Even if they aren't specifically modules, it will help ensure our tests match up to something. Eventually this should also help with pitest as everything will match up.

A UT should be created to make sure we don't break this in the future.

Examples of this being broken:

com\puppycrawl\tools\checkstyle\checks\FileSetCheckLifecycleTest.java
com\puppycrawl\tools\checkstyle\checks\imports\ImportControlRegExpInPkgTest.java
com\puppycrawl\tools\checkstyle\checks\imports\ImportControlRegExpTest.java
com\puppycrawl\tools\checkstyle\filefilters\ExclusionBeforeExecutionFileFilterTest.java
com\puppycrawl\tools\checkstyle\filters\BeforeExecutionFileFilterSetTest.java
com\puppycrawl\tools\checkstyle\filters\FilterSetTest.java
com\puppycrawl\tools\checkstyle\xpath\XpathMapperTest.java

ExclusionBeforeExecutionFileFilterTest is a UT for BeforeExecutionExclusionFileFilter.
BeforeExecutionFileFilterSetTest is in api not filters.
FilterSetTest is in api and not filters.
ImportControlRegExpInPkgTest, and ImportControlRegExpTest are UTs for ImportControl.
XpathMapperTest is more a UT for RootNode and all Xpath functionality really.

This is a list of tests that should be in a special package (probably internal) for helper files, and such:

com\puppycrawl\tools\checkstyle\AuditEventUtFormatter.java
com\puppycrawl\tools\checkstyle\BriefUtLogger.java
com\puppycrawl\tools\checkstyle\CheckerStub.java
com\puppycrawl\tools\checkstyle\CloseAndFlushTestByteArrayOutputStream.java
com\puppycrawl\tools\checkstyle\DebugAuditAdapter.java
com\puppycrawl\tools\checkstyle\DebugFilter.java
com\puppycrawl\tools\checkstyle\TestBeforeExecutionFileFilter.java
com\puppycrawl\tools\checkstyle\TestLoggingReporter.java
com\puppycrawl\tools\checkstyle\TestRootModuleChecker.java
@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Sep 13, 2017

Member

@romani

ImportControlRegExpInPkgTest, and ImportControlRegExpTest are UTs for ImportControl.

Are we ok with test classes with many test methods? I'm sure we have a check against too many methods in a class somewhere.
We already have a ImportControlTest so we would need to combine these 3 files into 1.

XpathMapperTest is more a UT for RootNode and all Xpath functionality really.

I didn't look at existing tests when making the list in main description, but it looks like we already have a RootNodeTest.
Looking at the class more, this seems like a test of XPathEvaluator (which is not our code) and verifying full regression of everything xpath.
Does it make more sense for this to be in it's own special package, or in the XPathEvaluator package, or just exclude it from the UT and leave it where it is? Maybe rename to XPathEvaluatorTest?

@MEZk Feel free to share your input here since you are the project lead.

Member

rnveach commented Sep 13, 2017

@romani

ImportControlRegExpInPkgTest, and ImportControlRegExpTest are UTs for ImportControl.

Are we ok with test classes with many test methods? I'm sure we have a check against too many methods in a class somewhere.
We already have a ImportControlTest so we would need to combine these 3 files into 1.

XpathMapperTest is more a UT for RootNode and all Xpath functionality really.

I didn't look at existing tests when making the list in main description, but it looks like we already have a RootNodeTest.
Looking at the class more, this seems like a test of XPathEvaluator (which is not our code) and verifying full regression of everything xpath.
Does it make more sense for this to be in it's own special package, or in the XPathEvaluator package, or just exclude it from the UT and leave it where it is? Maybe rename to XPathEvaluatorTest?

@MEZk Feel free to share your input here since you are the project lead.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Sep 13, 2017

Contributor

Are we ok with test classes with many test methods?

I'm against such tests. I think that the tests should be split.

I'm sure we have a check against too many methods in a class somewhere.

The check should be enforsed over test classes, I guess.

Looking at the class more, this seems like a test of XPathEvaluator

Not exactly. It is a kind of test which verifies that all logic which maps DetailAST tree to Xpath-tree works OK. In addition it might be a siginificat source of information for Xpath suppression model users. Maybe it is reasonable to move the test to IT folder.

Does it make more sense for this to be in it's own special package

Yes.

Contributor

MEZk commented Sep 13, 2017

Are we ok with test classes with many test methods?

I'm against such tests. I think that the tests should be split.

I'm sure we have a check against too many methods in a class somewhere.

The check should be enforsed over test classes, I guess.

Looking at the class more, this seems like a test of XPathEvaluator

Not exactly. It is a kind of test which verifies that all logic which maps DetailAST tree to Xpath-tree works OK. In addition it might be a siginificat source of information for Xpath suppression model users. Maybe it is reasonable to move the test to IT folder.

Does it make more sense for this to be in it's own special package

Yes.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 14, 2017

Member

Are we ok with test classes with many test methods?

this is interesting point of discussion.
It is not good. But ..... to find a test in several test even worse probelem.
It will be hard to make decision where to put new test method and ....
For now I allow this and think we do give more benefits from it than having problems.
If smb have ideas on how to make a split please share.

Member

romani commented Sep 14, 2017

Are we ok with test classes with many test methods?

this is interesting point of discussion.
It is not good. But ..... to find a test in several test even worse probelem.
It will be hard to make decision where to put new test method and ....
For now I allow this and think we do give more benefits from it than having problems.
If smb have ideas on how to make a split please share.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 14, 2017

Member

@rnveach and @MEZk

XpathMapperTest is more a UT for RootNode and all Xpath functionality really.

There is no XpathMapper in our code. XPathEvaluator that is used in internals is class from library(saxon). So we have a case where we have a Test class for non existing "main" class in our project. That is ok and in real life happens a lot.
As we become to have some rules ..... it become a bit complicated with pros and cons.

RootNode (from main area of code) is most close class that is heavy used XpathMapperTest.
So it makes "some" sense to to append test to RootNode.
In all other parts of test code we do not use pure UT methodology as in my experience pure UT are useless, so in UT are awe write more functional tests that do testing by running almost whole library.
That give a lot of benefits.
I do not see big harm from moving all test to RootNodeTest but it will be better to put it to XpathFilterTest.
If we enforce rule that each main class have same named test class, we will simplify user ability to find proper test. For new contributor, to XpathMapperTest will be more complicated.

Maybe it is reasonable to move the test to IT folder.

IT test code is another mis-usage by us from general meaning of this area.
https://maven.apache.org/guides/introduction/introduction-to-the-lifecycle.html#Lifecycle_Reference
sequence of maven phases: "..., package, pre-integration-test ,integration-test , post-integration-test, verify ..."
IT test are suppose to test created jar (phase "package"). In our scope it should be testing of our plugins for compatibility or whole library execution (that we do CIs now).
I used IT area when Google style guide is introduced and we needed to keep UTs for style in some area. I did not wanted to pollute "test" are of code.
So I make interpretation like this:
UT is for any custom testing of Check.
IT is for testing on strict configuration (google style, sun style, ...... ) where user can see examples of valid code and invalid code. As all style guides are lacking examples for conner cases.

Member

romani commented Sep 14, 2017

@rnveach and @MEZk

XpathMapperTest is more a UT for RootNode and all Xpath functionality really.

There is no XpathMapper in our code. XPathEvaluator that is used in internals is class from library(saxon). So we have a case where we have a Test class for non existing "main" class in our project. That is ok and in real life happens a lot.
As we become to have some rules ..... it become a bit complicated with pros and cons.

RootNode (from main area of code) is most close class that is heavy used XpathMapperTest.
So it makes "some" sense to to append test to RootNode.
In all other parts of test code we do not use pure UT methodology as in my experience pure UT are useless, so in UT are awe write more functional tests that do testing by running almost whole library.
That give a lot of benefits.
I do not see big harm from moving all test to RootNodeTest but it will be better to put it to XpathFilterTest.
If we enforce rule that each main class have same named test class, we will simplify user ability to find proper test. For new contributor, to XpathMapperTest will be more complicated.

Maybe it is reasonable to move the test to IT folder.

IT test code is another mis-usage by us from general meaning of this area.
https://maven.apache.org/guides/introduction/introduction-to-the-lifecycle.html#Lifecycle_Reference
sequence of maven phases: "..., package, pre-integration-test ,integration-test , post-integration-test, verify ..."
IT test are suppose to test created jar (phase "package"). In our scope it should be testing of our plugins for compatibility or whole library execution (that we do CIs now).
I used IT area when Google style guide is introduced and we needed to keep UTs for style in some area. I did not wanted to pollute "test" are of code.
So I make interpretation like this:
UT is for any custom testing of Check.
IT is for testing on strict configuration (google style, sun style, ...... ) where user can see examples of valid code and invalid code. As all style guides are lacking examples for conner cases.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Sep 14, 2017

Member

If smb have ideas on how to make a split please share.

I don't. Right now I prefer everything in one class. Finding test that hits specific code is easier as you just run class tests and set a breakpoint. It is rare, in checkstyle, that I ever am looking for a specific test over looking for something that hits a specific line in production code.
If test class does become too big, maybe it is really a sign that the production class does too much...

XpathMapperTest ... it will be better to put it to XpathFilterTest.

To me, this seems like a weird place to put it as XpathMapperTest doesn't hit any coverage in XpathFilter. Usually when we add new tests, it is for the class that impacts it and it's functionality.
Coverage says this test hits ElementNode and RootNode the most, 97% and 81% respectively.
XPathEvaluator and XPathExpression are covered by 71% and 52% respectively.

harm from moving all test to RootNodeTest

I am fine with this or something under XPathEvaluator/XPathExpression.

Member

rnveach commented Sep 14, 2017

If smb have ideas on how to make a split please share.

I don't. Right now I prefer everything in one class. Finding test that hits specific code is easier as you just run class tests and set a breakpoint. It is rare, in checkstyle, that I ever am looking for a specific test over looking for something that hits a specific line in production code.
If test class does become too big, maybe it is really a sign that the production class does too much...

XpathMapperTest ... it will be better to put it to XpathFilterTest.

To me, this seems like a weird place to put it as XpathMapperTest doesn't hit any coverage in XpathFilter. Usually when we add new tests, it is for the class that impacts it and it's functionality.
Coverage says this test hits ElementNode and RootNode the most, 97% and 81% respectively.
XPathEvaluator and XPathExpression are covered by 71% and 52% respectively.

harm from moving all test to RootNodeTest

I am fine with this or something under XPathEvaluator/XPathExpression.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 15, 2017

Member

@MEZk , your turn to support your ideas

Member

romani commented Sep 15, 2017

@MEZk , your turn to support your ideas

romani added a commit that referenced this issue Sep 15, 2017

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

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 16, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 24, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 24, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 24, 2017

rnveach added a commit that referenced this issue Sep 25, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Sep 25, 2017

Member

@MEZk @romani I have not received word on how we plan to handle ImportControlRegExpInPkgTest, ImportControlRegExpTest, and XpathMapperTest.
I will be starting a PR soon.

Member

rnveach commented Sep 25, 2017

@MEZk @romani I have not received word on how we plan to handle ImportControlRegExpInPkgTest, ImportControlRegExpTest, and XpathMapperTest.
I will be starting a PR soon.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 25, 2017

Member

@MEZk , we are waiting for you to reply.
If you do not have goos reasons to keep code as is or no time to continue discussion, lets agree on our proposal to unblock rnveach.

Member

romani commented Sep 25, 2017

@MEZk , we are waiting for you to reply.
If you do not have goos reasons to keep code as is or no time to continue discussion, lets agree on our proposal to unblock rnveach.

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 26, 2017

romani added a commit that referenced this issue Sep 28, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Oct 25, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Oct 25, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Oct 25, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Oct 25, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Oct 25, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Oct 25, 2017

romani added a commit that referenced this issue Oct 26, 2017

@romani romani modified the milestones: 8.3, 8.4 Oct 26, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 26, 2017

Member

final fix is merged.

Member

romani commented Oct 26, 2017

final fix is merged.

@romani romani closed this Oct 26, 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

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