Module term and usage is confusing in test area #3667

Closed
rnveach opened this Issue Dec 19, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@rnveach
Member

rnveach commented Dec 19, 2016

We have a utility method getCheckstyleModules, which I thought would return every module we have in checkstyle. Isn't everything we do a module?

isCheckstyleModule does not define AbstractFileSetCheck. Isn't that a module? We define filters, filefilters, and checks. Why isn't AbstractFileSetCheck a module? We also don't define root module Checker.

isCheckstyleNonAbstractCheck limits only checks that end with Check, but we have checks that don't end with that. Example is FileContentsHolder and SuppressWarningsHolder.

In testAllCheckstyleModulesInCheckstyleConfig we are trying to compare checks to modules. The test is named module so we should be checking all modules, right? Not all checks.

In testAllCheckstyleModulesHaveMessage we are checking only checks but we named the test module.

http://checkstyle.sourceforge.net/config.html#Modules

Root module Checker has child FileSetChecks JavadocPackage and TreeWalker.
Module TreeWalker has submodules AvoidStarImport, ConstantName, and EmptyBlock.

Everything we do is a module by these definitions. So getCheckstyleModules should return everything, and all tests that rely on it should be changed to better suite what they are testing.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 19, 2016

Member

Isn't everything we do a module?

If in config it is module so it is a module.

AbstractFileSetCheck. Isn't that a module?

It is not, only exact implementations that could be instaciated could be a module.

The test is named module so we should be checking all modules, right? Not all checks

Yes. Or rename the method.

So getCheckstyleModules should return everything, and all tests that rely on it should be changed to better suite what they are testing.

If that is easy, yes. But we could rename it too.

Member

romani commented Dec 19, 2016

Isn't everything we do a module?

If in config it is module so it is a module.

AbstractFileSetCheck. Isn't that a module?

It is not, only exact implementations that could be instaciated could be a module.

The test is named module so we should be checking all modules, right? Not all checks

Yes. Or rename the method.

So getCheckstyleModules should return everything, and all tests that rely on it should be changed to better suite what they are testing.

If that is easy, yes. But we could rename it too.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 19, 2016

Member

AbstractFileSetCheck. Isn't that a module?

It is not, only exact implementations that could be instantiated

Sorry for the confusion, I meant any type that extends AbstractFileSetCheck. For example, TreeWalker is not currently picked up as a module.

Member

rnveach commented Dec 19, 2016

AbstractFileSetCheck. Isn't that a module?

It is not, only exact implementations that could be instantiated

Sorry for the confusion, I meant any type that extends AbstractFileSetCheck. For example, TreeWalker is not currently picked up as a module.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 19, 2016

Member

Yes. Agree.
All classes that could be referenced in config are modules.

Member

romani commented Dec 19, 2016

Yes. Agree.
All classes that could be referenced in config are modules.

rnveach added a commit to rnveach/checkstyle that referenced this issue Dec 19, 2016

romani added a commit that referenced this issue Dec 20, 2016

@romani romani added this to the 7.4 milestone Dec 20, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 20, 2016

Member

@rnveach , PR is merged.
please close it if addressed all points that you raised int his issue.

Member

romani commented Dec 20, 2016

@rnveach , PR is merged.
please close it if addressed all points that you raised int his issue.

@rnveach rnveach closed this Dec 20, 2016

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