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

New CheckstyleTestMakeupCheck #610

Closed
rnveach opened this issue Sep 28, 2017 · 4 comments
Closed

New CheckstyleTestMakeupCheck #610

rnveach opened this issue Sep 28, 2017 · 4 comments
Labels
Milestone

Comments

@rnveach
Copy link
Contributor

rnveach commented Sep 28, 2017

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

We need a new check to bring uniformity to our UTs in Checkstyle.
There are so many ways to write a test that some conflict with our custom check in regression to read these checks easily.

  1. Some properties are defined with variables:
    https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/checks/naming/AbstractClassNameCheckTest.java#L66
  2. Some check configurations are defined as a class field and some are defined as local variables:
    https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/DefaultComesLastCheckTest.java#L43

I am currently naming the check CheckstyleTestMakeupCheck and started a preview of the check at https://github.com/rnveach/checkstyle/commits/regression_util.

We should make a check that only examines methods defined with @Test and have a verify method call.
The method must have a local variable defining a DefaultConfiguration and assigning it with either createModuleConfig(...) or one of our other standard configuration creation methods.
If it calls addAttribute on the DefaultConfiguration the property name must be a string, and the property value must either be: a String, a concatentation of 2 or more strings, a enumeration toString in the form XXX.YYY.toString(), getPath(String), or the literal null (which is technically not supported in the configuration file).

@romani Please review and approve.
Let me know if you can think of anything else to verify.

@romani
Copy link
Member

romani commented Oct 3, 2017

If it can resolve problems in our code we are ok to have it, even it is very specific to our repo and nobody will use it expect for us.

@rnveach
Copy link
Contributor Author

rnveach commented Nov 15, 2017

Check needs to be changed to understand tests with multiple configurations in 1 test.
Example:
https://github.com/checkstyle/checkstyle/blob/2aca36156fa461f2d54f1386743a0146bdf8b4bd/src/test/java/com/puppycrawl/tools/checkstyle/checks/header/HeaderCheckTest.java#L236-L247
1 test defines custom HeaderCheck, and defines custom Checker.
We need to distinguish between them and make sure all are configured correctly from each other.

@rnveach
Copy link
Contributor Author

rnveach commented Nov 25, 2017

Check was merged.

@rnveach rnveach closed this as completed Nov 25, 2017
@rnveach rnveach added this to the 1.25.0 milestone Nov 25, 2017
@rnveach
Copy link
Contributor Author

rnveach commented Nov 25, 2017

@romani Feel free to release sevntu whenever you can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants