@Luolc Remember to ping me when you have made fixes and reply asDone to each point, especially this discussion#85 (comment).
We don't get notifications when new commits have been pushed.

@rnveach
I am still working on#85 (comment) and was thinking to @ you when finish this together. And I found I might spend more time so I decided commit part of the work yesterday.

I scan and output all the property info from UTs, and I have to check them manually whether the processor missed anything or produce anything wrong.

I am working on the config as class field and the enum property value, but I am still not very confident that they are absolutely good now.

I added some reply and others are still working. Checking the result of all checkstyle module UTs may still take a lot of time I think.

Checking the result of all checkstyle module UTs may still take a lot of time I think.

I would try automating it somehow and not worry about manually checking every single digit.

For example, I said if you run your check does any module file not produce any results?
We have reflection code that lists all module properties. Use that to against the results of your check to see if any properties were missed or if any extra were picked up.
Put breaks in your check to see what type of property values are not being selected, but match the other criteria.

I already gave you some examples of other areas that your check doesn't cover, so we can go with that for now. Those should be the most popular ways to code the tests.
Lets not waste time being this precise for now. Usage should show us more uncommon cases in the future.

@Luolc ping

@rnveach
There are still many modules left now.
I wrote this for helping me analyze the UTs:https://github.com/Luolc/regression-tool/blob/ut-scan/src/test/java/com/github/checkstyle/regression/customcheck/UnitTestProcessorCheckTest.java#L101. I checked by the assertion and break points in the code.

I made some updates and could also support value as:

• Strings joining with+, like"CLASS_DEF" + ",VARIABLE_DEF"
• Enums likeBlockOption.TEXT.toString(). Here I used regex to handle this. So it might have problems if the expression is in multiple lines.
• module config declared as class field, andassigned in the single UT method. I would explain this at below.

The problems found currently:
1.BeforeExecutionExclusionFileFilter: the UT class name is notBeforeExecutionExclusionFileFilterTest butExclusionBeforeExecutionFileFilterTest.

2.The property value could be a variable:
current tests found:

• AbstractClassNameCheck
• AtclauseOrderCheck
• CatchParameterNameCheck

example:https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/checks/naming/AbstractClassNameCheckTest.java#L66

3.The property value could be a variable:
current tests found:

• HeaderCheck

example:https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/checks/header/HeaderCheckTest.java#L63

4.Some UTs missed tests of some properties
current tests found:

• ClassFanOutComplexityCheck
• ConstantNameCheck

example:https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/checks/naming/ConstantNameCheckTest.java , no test for propertyapplyToPrivate

5.different way of class field module config
I mentioned above that we could support a kind of class field config now.
example:https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/DefaultComesLastCheckTest.java#L43
The variable could be declared as class field,but we must re-assign it every time in each UT method

There are many other UTs with class field config that is assign insetUp. I haven't done with this yet.

6.Some UTs don't usecreateModuleConfig

• FileTabCharacterCheck: createConfig
• BeforeExecutionExclusionFileFilter: createBeforeExecutionFileFilterConfig

we may need to collect names of all these kind of method.

------
My checking is atHeaderCheck now (in a Lexicographic order). 40 modules. While we have 160+ modules, there are still too many. And I have already found a lot of different kind of situation that cannot support currently. I am afraid I still need much time on this if we want to solve them.
This is the list of them:

com.puppycrawl.tools.checkstyle.checks.naming.AbbreviationAsWordInNameCheckcom.puppycrawl.tools.checkstyle.checks.naming.AbstractClassNameCheckcom.puppycrawl.tools.checkstyle.checks.annotation.AnnotationLocationCheckcom.puppycrawl.tools.checkstyle.checks.annotation.AnnotationUseStyleCheckcom.puppycrawl.tools.checkstyle.checks.sizes.AnonInnerLengthCheckcom.puppycrawl.tools.checkstyle.checks.coding.ArrayTrailingCommaCheckcom.puppycrawl.tools.checkstyle.checks.ArrayTypeStyleCheckcom.puppycrawl.tools.checkstyle.checks.javadoc.AtclauseOrderCheckcom.puppycrawl.tools.checkstyle.checks.AvoidEscapedUnicodeCharactersCheckcom.puppycrawl.tools.checkstyle.checks.coding.AvoidInlineConditionalsCheckcom.puppycrawl.tools.checkstyle.checks.blocks.AvoidNestedBlocksCheckcom.puppycrawl.tools.checkstyle.checks.imports.AvoidStarImportCheckcom.puppycrawl.tools.checkstyle.checks.imports.AvoidStaticImportCheckcom.puppycrawl.tools.checkstyle.checks.metrics.BooleanExpressionComplexityCheckcom.puppycrawl.tools.checkstyle.checks.naming.CatchParameterNameCheckcom.puppycrawl.tools.checkstyle.checks.metrics.ClassDataAbstractionCouplingCheckcom.puppycrawl.tools.checkstyle.checks.naming.ClassTypeParameterNameCheckcom.puppycrawl.tools.checkstyle.checks.indentation.CommentsIndentationCheckcom.puppycrawl.tools.checkstyle.checks.coding.CovariantEqualsCheckcom.puppycrawl.tools.checkstyle.checks.imports.CustomImportOrderCheckcom.puppycrawl.tools.checkstyle.checks.metrics.CyclomaticComplexityCheckcom.puppycrawl.tools.checkstyle.checks.coding.DeclarationOrderCheckcom.puppycrawl.tools.checkstyle.checks.coding.DefaultComesLastCheckcom.puppycrawl.tools.checkstyle.checks.DescendantTokenCheckcom.puppycrawl.tools.checkstyle.checks.design.DesignForExtensionCheckcom.puppycrawl.tools.checkstyle.checks.blocks.EmptyBlockCheckcom.puppycrawl.tools.checkstyle.checks.blocks.EmptyCatchBlockCheckcom.puppycrawl.tools.checkstyle.checks.whitespace.EmptyLineSeparatorCheckcom.puppycrawl.tools.checkstyle.checks.coding.EmptyStatementCheckcom.puppycrawl.tools.checkstyle.checks.coding.EqualsAvoidNullCheckcom.puppycrawl.tools.checkstyle.checks.coding.EqualsHashCodeCheckcom.puppycrawl.tools.checkstyle.checks.sizes.ExecutableStatementCountCheckcom.puppycrawl.tools.checkstyle.checks.coding.ExplicitInitializationCheckcom.puppycrawl.tools.checkstyle.checks.coding.FallThroughCheckcom.puppycrawl.tools.checkstyle.checks.sizes.FileLengthCheckcom.puppycrawl.tools.checkstyle.checks.design.FinalClassCheckcom.puppycrawl.tools.checkstyle.checks.coding.FinalLocalVariableCheckcom.puppycrawl.tools.checkstyle.checks.FinalParametersCheckcom.puppycrawl.tools.checkstyle.checks.whitespace.GenericWhitespaceCheckcom.puppycrawl.tools.checkstyle.checks.header.HeaderCheck

Enums like BlockOption.TEXT.toString(). Here I used regex to handle this. So it might have problems if the expression is in multiple lines.

Why regular expression? If it is in the formXXX.YYY.toString() can't we assume what we want is the IDENTYYY?

BeforeExecutionExclusionFileFilter: the UT class name is not BeforeExecutionExclusionFileFilterTest but ExclusionBeforeExecutionFileFilterTest.

I seefilefilters.BeforeExecutionExclusionFileFilter in production, andfilters.BeforeExecutionFileFilterSetTest andfilefilters.ExclusionBeforeExecutionFileFilterTest in test.
Let's make an issue of this in Checkstyle. We need tests named after their classes they are testing. Something should be namedBeforeExecutionExclusionFileFilterTest.

2. The property value could be a variable

Let's split this off into another issue. The example you gave is pretty simple and the variable could be inlined. The check could also be modified to just track variable assignments and pickup the very basic uses.

3.The property value could be a variable

Move this to another issue too. I think we should recognizegetPath("String") as we need something to run regression on when the input is a file.

Some UTs missed tests of some properties

This must be an issue in Checkstyle.
There is nothing we can do if they miss UT on their own properties.
We will still pick up property in#84 , but picking a value to use for it might be harder. If we have a property to use in the configuration, but can't decide on a value, we should print some type of warning in the console of our application and drop the property.
For CIs we can make an additional option to fail the build if this happens.

6.Some UTs don't use createModuleConfig

I still think we should create an issue in Checkstyle to have them organize their UTs more.

FileTabCharacterCheck: createConfig
BeforeExecutionExclusionFileFilter: createBeforeExecutionFileFilterConfig

Especially these 2. I don't see a reason right now why they can't be standardized.

we may need to collect names of all these kind of method.

Yes, we will need to do this for any issues we create. It would be better if we could atleast create an automated way to detect these instances and not rely on manually reading each one.
You can atleast start the issues and expand on them as more is found.

@Luolc

It would be better if we could atleast create an automated way to detect these instances and not rely on manually reading each one.

Is it possible to automate finding of bad cases, even if it only finds like 80% of the cases?
For example:
if a UT method (with annotation@Test) hasverify method call, then it must have variable definitionDefaultConfiguration XXX and if it doesXXX.addAttribute then the parameters must be a String,getPath("String"), or an enum in the form we have discussed above.

Automating the finding of bad cases would allow us to finish this specific issue faster and move work to fixing up Checkstyle's sources while we incorporate the data found here for more work we need to do in our logic.

Why regular expression? If it is in the form XXX.YYY.toString() can't we assume what we want is the IDENT YYY?

I don't know if we have smth likeXXX.YYY.ZZZ.toString(), or full qualified name ones. Regex could handle them instead. If we finally make sure that we only have one format, then getting the value from ast could be better.

Let's split this off into another issue. The example you gave is pretty simple and the variable could be inlined. The check could also be modified to just track variable assignments and pickup the very basic uses.

OK. That makes sense.

Move this to another issue too. I think we should recognize getPath("String") as we need something to run regression on when the input is a file.

AlthoughgetPath is a method of super class. It is possible to be override anyway.

Is it possible to automate finding of bad cases, even if it only finds like 80% of the cases?

Maybe I need another custom check, or at least some kind of regex way for this.
My current way is still low-efficient.

I would sum up the issues we need to create both here and in checkstyle repo and create them together sooner.

Maybe I need another custom check

yes, it would need to be in sevntu as it is just for us.
https://github.com/sevntu-checkstyle/sevntu.checkstyle

Although getPath is a method of super class. It is possible to be override anyway.

Yes, to get the correct path would would have to inspect thegetPackageLocation method which should always be a simplereturn "string" now that we are rewriting tests some.

I don't know if we have smth like XXX.YYY.ZZZ.toString()

I doubt it, but something like the custom check I mentioned in previous post to validate UTs that useverify would find out if there was one like this pretty fast as it will print a violation on it.

I think it makes more and more sense we go this route and force checkstyle use the styling variants we approve. This will make things easier on our custom check for all the weird cases we could possibly see.

@Luolc ping

1.BeforeExecutionExclusionFileFilter: the UT class name is not BeforeExecutionExclusionFileFilterTest but ExclusionBeforeExecutionFileFilterTest.

Issue #5104 in checkstyle was created for this.

Point 1 is nearly fixed in Checkstylecheckstyle/checkstyle#5104 .
Point 2 and 5 is now an issue in Sevntusevntu-checkstyle/sevntu.checkstyle#610.
Point 3 should just be a case we need to support. All path locations have been standardized in Checkstyle, so we should be able to find the input pretty easily.
Point 4 is now an issue in Checkstylecheckstyle/checkstyle#5156 .
Point 6 is now an issue in Checkstylecheckstyle/checkstyle#5157.

This PR is on hold until the issues are fixed and uniformity is brought to Checkstyle.
We should be able to use this check to verify issues in Checkstyle are fixed.

Custom check needs to be changed to gather information from 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 the Check we want and others. I'm not sure if we can get all values or should.

New check is done and will soon be added to Checkstyle.

Points 1, 2, 5, and 6 are done.
Point 4 can be done at any time.

All that is left is to fix this PR and merge it.