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

Comments

Projects
None yet
2 participants
@rnveach
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

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 3, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 15, 2017

Contributor

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.

Contributor

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 added a commit to rnveach/sevntu.checkstyle that referenced this issue Nov 16, 2017

rnveach added a commit to rnveach/sevntu.checkstyle that referenced this issue Nov 16, 2017

rnveach added a commit to rnveach/sevntu.checkstyle that referenced this issue Nov 17, 2017

rnveach added a commit to rnveach/sevntu.checkstyle that referenced this issue Nov 24, 2017

rnveach added a commit to rnveach/sevntu.checkstyle that referenced this issue Nov 24, 2017

rnveach added a commit to rnveach/sevntu.checkstyle that referenced this issue Nov 24, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 25, 2017

Contributor

Check was merged.

Contributor

rnveach commented Nov 25, 2017

Check was merged.

@rnveach rnveach closed this Nov 25, 2017

@rnveach rnveach added this to the 1.25.0 milestone Nov 25, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 25, 2017

Contributor

@romani Feel free to release sevntu whenever you can.

Contributor

rnveach commented Nov 25, 2017

@romani Feel free to release sevntu whenever you can.

rnveach added a commit that referenced this issue Nov 26, 2017

kariem added a commit to kariem/sevntu.checkstyle that referenced this issue Jul 26, 2018

kariem added a commit to kariem/sevntu.checkstyle that referenced this issue Jul 26, 2018

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