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

Issue #79: Processing UT changes to get possible property values #85

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Luolc
Copy link
Contributor

@Luolc Luolc commented Aug 21, 2017

#79

This PR includes a custom check and the utility class to run the check.

The check walk through all @Test annotation to target the UT method, find the statement like final DefaultConfiguration fooModuleConfig = createModuleConfig(FooCheck.class), and get properties by detecting the fooModuleConfig.addAttribute method call.

If this is OK, then we could integrate this into our generator.

// Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
////////////////////////////////////////////////////////////////////////////////

package com.github.checkstyle.regression.module;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we put custom checks in a new package?
Something like customcheck.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

import com.puppycrawl.tools.checkstyle.api.TokenTypes;

/**
* The custom check which processes the unit test class of a checkstyle module.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we 'process' the check, but it doesn't specify what it grabs from said processing.
First sentence should be a quick summary, 2nd+ sentences should talk about details.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

import com.puppycrawl.tools.checkstyle.api.TokenTypes;

/**
* The custom check which processes the unit test class of a checkstyle module.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add extra blank line between summary and details.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

* This check would walk through the {@code @Test} annotation, find variable definition
* like {@code final DefaultConfiguration checkConfig = createModuleConfig(FooCheck.class)}
* and grab the property info from {@link DefaultConfiguration#addAttribute(String, String)}
* method call.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we won't support tests where checkConfig is defined as a field, instead of a local variable?
It is not possible to support both?

If so, we need changes in Checkstyle as there are loads of different ways tests are made and they must be organized.
Please make a new issue in checkstyle and let's get @romani 's approval.

If we are going this way, please update Javadoc with example of how test classes should be defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we won't support tests where checkConfig is defined as a field, instead of a local variable?
It is not possible to support both?

I think it could be possible. I didn't realize the config could be a field before. I would have a try on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Luolc Please use your check and scan Checkstyle's repo. Any true module tests where your check finds nothing or not enough is an area you need to look into.

final List<File> processedFiles = Collections.singletonList(new File(path));
checker.process(processedFiles);

final UnitTestProcessorCheck check = getCheckInstance(checker);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Truthfully we don't need Check's instance. We just need unitTestToPropertiesMap.
Why don't we make field static and create a getter for it? Than we can just do UnitTestProcessorCheck.getUnitTestToPropertiesMap instead of doing reflection hacks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to static method now.

import com.puppycrawl.tools.checkstyle.api.TokenTypes;
import com.puppycrawl.tools.checkstyle.utils.CommonUtils;

public class ReturnCountCheckTest extends AbstractModuleTestSupport {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This confused me at first as I was expecting a test file for UnitTestProcessorCheck and was wondering where ReturnCount came from.
Does this need to be ReturnCount? Can we turn it into some dummy name like InputModuleExampleTest?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. This class is a copy of ReturnCountCheckTest from checkstyle repo. I didn't change its name at first.

* @throws CheckstyleException failure when running the check
* @throws ReflectiveOperationException failure of reflection on checker and tree walker
*/
public static Map<String, Set<ModuleInfo.Property>> process(String path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class has 2 functions, executing a custom check and getting the results for the specific check.
I assume we may need to write another custom check for #6 .

If you agree, I think we should rename this class to something more generic, have it take the module class as input, and return nothing as everything will be retrieved through static getters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

// Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
////////////////////////////////////////////////////////////////////////////////

package com.github.checkstyle.regression.module;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should move with the custom check too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

config/pmd.xml Outdated
<rule ref="rulesets/java/comments.xml/CommentRequired" />
<rule ref="rulesets/java/comments.xml/CommentRequired">
<properties>
<property name="publicMethodCommentRequirement" value="Ignored"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain this change.

pom.xml Outdated
@@ -394,6 +394,11 @@
<lineRate>0</lineRate>
</regex>
<regex>
<pattern>com.github.checkstyle.regression.module.UnitTestProcessorCheck</pattern>
<branchRate>76</branchRate>
<lineRate>92</lineRate>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you are making changes, please post this file's code coverage report.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not done.

@Luolc Luolc force-pushed the issue79 branch 2 times, most recently from f66cd92 to 9b01307 Compare August 24, 2017 08:38
@rnveach
Copy link
Member

rnveach commented Aug 24, 2017

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

<rule ref="rulesets/java/comments.xml/CommentRequired" />
<rule ref="rulesets/java/comments.xml/CommentRequired">
<properties>
<property name="violationSuppressXPath" value="//Annotation/MarkerAnnotation//Name[@Image='Override']"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this pmd would force us to add javadoc on @Override method. I copied this settings from checkstyle repo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks.
This can be ignored.

pom.xml Outdated
@@ -394,6 +394,11 @@
<lineRate>0</lineRate>
</regex>
<regex>
<pattern>com.github.checkstyle.regression.module.UnitTestProcessorCheck</pattern>
<branchRate>76</branchRate>
<lineRate>92</lineRate>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not done.

Copy link
Member

@rnveach rnveach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-verifying items that are now shown as hidden.

}

/**
* Processes the file with the given path using the given custom check.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update documentation that check needs a public/static way to retrieve it's own results using this method.

* <p>This check would walk through the {@code @Test} annotation, find variable definition
* like {@code final DefaultConfiguration checkConfig = createModuleConfig(FooCheck.class)}
* and grab the property info from {@link DefaultConfiguration#addAttribute(String, String)}
* method call.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#85 (comment)

If we are going this way, please update Javadoc with example of how test classes should be defined.

Not done.

final DetailAST methodDef = ast.getParent().getParent();
final DetailAST methodBlock = methodDef.findFirstToken(TokenTypes.SLIST);
final Optional<String> configVariableName =
getModuleConfigVariableName(methodBlock);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#85 (comment)

So we won't support tests where checkConfig is defined as a field, instead of a local variable?
It is not possible to support both?

I don't see an update or any tests for this.

@Luolc
Copy link
Contributor Author

Luolc commented Aug 25, 2017

@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.

@rnveach
Copy link
Member

rnveach commented Aug 25, 2017

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.

@rnveach
Copy link
Member

rnveach commented Aug 28, 2017

@Luolc ping

@Luolc
Copy link
Contributor Author

Luolc commented Aug 28, 2017

@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 like BlockOption.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, and assigned in the single UT method. I would explain this at below.

The problems found currently:
1.BeforeExecutionExclusionFileFilter: the UT class name is not BeforeExecutionExclusionFileFilterTest but ExclusionBeforeExecutionFileFilterTest.

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 property applyToPrivate

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 in setUp. I haven't done with this yet.

6.Some UTs don't use createModuleConfig

  • FileTabCharacterCheck: createConfig
  • BeforeExecutionExclusionFileFilter: createBeforeExecutionFileFilterConfig

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

------
My checking is at HeaderCheck 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.AbbreviationAsWordInNameCheck
com.puppycrawl.tools.checkstyle.checks.naming.AbstractClassNameCheck
com.puppycrawl.tools.checkstyle.checks.annotation.AnnotationLocationCheck
com.puppycrawl.tools.checkstyle.checks.annotation.AnnotationUseStyleCheck
com.puppycrawl.tools.checkstyle.checks.sizes.AnonInnerLengthCheck
com.puppycrawl.tools.checkstyle.checks.coding.ArrayTrailingCommaCheck
com.puppycrawl.tools.checkstyle.checks.ArrayTypeStyleCheck
com.puppycrawl.tools.checkstyle.checks.javadoc.AtclauseOrderCheck
com.puppycrawl.tools.checkstyle.checks.AvoidEscapedUnicodeCharactersCheck
com.puppycrawl.tools.checkstyle.checks.coding.AvoidInlineConditionalsCheck
com.puppycrawl.tools.checkstyle.checks.blocks.AvoidNestedBlocksCheck
com.puppycrawl.tools.checkstyle.checks.imports.AvoidStarImportCheck
com.puppycrawl.tools.checkstyle.checks.imports.AvoidStaticImportCheck
com.puppycrawl.tools.checkstyle.checks.metrics.BooleanExpressionComplexityCheck
com.puppycrawl.tools.checkstyle.checks.naming.CatchParameterNameCheck
com.puppycrawl.tools.checkstyle.checks.metrics.ClassDataAbstractionCouplingCheck
com.puppycrawl.tools.checkstyle.checks.naming.ClassTypeParameterNameCheck
com.puppycrawl.tools.checkstyle.checks.indentation.CommentsIndentationCheck
com.puppycrawl.tools.checkstyle.checks.coding.CovariantEqualsCheck
com.puppycrawl.tools.checkstyle.checks.imports.CustomImportOrderCheck
com.puppycrawl.tools.checkstyle.checks.metrics.CyclomaticComplexityCheck
com.puppycrawl.tools.checkstyle.checks.coding.DeclarationOrderCheck
com.puppycrawl.tools.checkstyle.checks.coding.DefaultComesLastCheck
com.puppycrawl.tools.checkstyle.checks.DescendantTokenCheck
com.puppycrawl.tools.checkstyle.checks.design.DesignForExtensionCheck
com.puppycrawl.tools.checkstyle.checks.blocks.EmptyBlockCheck
com.puppycrawl.tools.checkstyle.checks.blocks.EmptyCatchBlockCheck
com.puppycrawl.tools.checkstyle.checks.whitespace.EmptyLineSeparatorCheck
com.puppycrawl.tools.checkstyle.checks.coding.EmptyStatementCheck
com.puppycrawl.tools.checkstyle.checks.coding.EqualsAvoidNullCheck
com.puppycrawl.tools.checkstyle.checks.coding.EqualsHashCodeCheck
com.puppycrawl.tools.checkstyle.checks.sizes.ExecutableStatementCountCheck
com.puppycrawl.tools.checkstyle.checks.coding.ExplicitInitializationCheck
com.puppycrawl.tools.checkstyle.checks.coding.FallThroughCheck
com.puppycrawl.tools.checkstyle.checks.sizes.FileLengthCheck
com.puppycrawl.tools.checkstyle.checks.design.FinalClassCheck
com.puppycrawl.tools.checkstyle.checks.coding.FinalLocalVariableCheck
com.puppycrawl.tools.checkstyle.checks.FinalParametersCheck
com.puppycrawl.tools.checkstyle.checks.whitespace.GenericWhitespaceCheck
com.puppycrawl.tools.checkstyle.checks.header.HeaderCheck

@rnveach
Copy link
Member

rnveach commented Aug 28, 2017

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 form XXX.YYY.toString() can't we assume what we want is the IDENT YYY?

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

I see filefilters.BeforeExecutionExclusionFileFilter in production, and filters.BeforeExecutionFileFilterSetTest and filefilters.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 named BeforeExecutionExclusionFileFilterTest.

  1. 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 recognize getPath("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.

@rnveach
Copy link
Member

rnveach commented Aug 28, 2017

@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) has verify method call, then it must have variable definition DefaultConfiguration XXX and if it does XXX.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.

@Luolc
Copy link
Contributor Author

Luolc commented Aug 28, 2017

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 like XXX.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.

Although getPath 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.

@rnveach
Copy link
Member

rnveach commented Aug 28, 2017

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 the getPackageLocation method which should always be a simple return "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 use verify 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.

@rnveach
Copy link
Member

rnveach commented Aug 30, 2017

@Luolc ping

@rnveach
Copy link
Member

rnveach commented Sep 12, 2017

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

Issue #5104 in checkstyle was created for this.

@rnveach
Copy link
Member

rnveach commented Sep 28, 2017

Point 1 is nearly fixed in Checkstyle checkstyle/checkstyle#5104 .
Point 2 and 5 is now an issue in Sevntu sevntu-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 Checkstyle checkstyle/checkstyle#5156 .
Point 6 is now an issue in Checkstyle checkstyle/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.

@rnveach
Copy link
Member

rnveach commented Nov 15, 2017

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.

@rnveach
Copy link
Member

rnveach commented Nov 25, 2017

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.

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

Successfully merging this pull request may close these issues.

2 participants