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

Expand XPath IT Regression Testing #6207

Open
rnveach opened this issue Nov 10, 2018 · 15 comments

Comments

@rnveach
Copy link
Member

commented Nov 10, 2018

Identified at #6198 (comment) ,

We need to expand XPath IT Regression to all checks that support it.

All checks listed under Currently, filter supports the following checks must appear with regression tests in https://github.com/checkstyle/checkstyle/tree/master/src/it/java/org/checkstyle/suppressionxpathfilter to showcase suppression by XPath works for the check.
Every log statement must be tested from the Check, so if 1 check has 3 log statements, the XPath test must have 3 tests, 1 for each log statement.

Tests can be copy of examples from our normal test tier at https://github.com/checkstyle/checkstyle/tree/master/src/test/java/com/puppycrawl/tools/checkstyle/checks just smaller.

@mincong-h

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

Hi, is this issue still valid? I can contribute some of these missing tests.

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

It is still valid as long as we are missing these tests.
You are free to contribute.

@mincong-h

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Before moving further on this ticket, I just want to make an exhaustive list of checks to be tested. So that we are all aware of the test effort. They are extracted from https://checkstyle.sourceforge.io/config_filters.html#SuppressionXpathFilter_Description

  • AbstractClassName
  • AnonInnerLength
  • ArrayTypeStyle
  • AvoidInlineConditionals
  • AvoidNestedBlocks
  • BooleanExpressionComplexity
  • CatchParameterName
  • ClassDataAbstractionCoupling
  • ClassFanOutComplexity
  • ClassMemberImpliedModifier
  • ClassTypeParameterName
  • ConstantName
  • CovariantEquals
  • CyclomaticComplexity
  • DeclarationOrder
  • DefaultComesLast
  • DescendantToken
  • DesignForExtension
  • EmptyBlock
  • EmptyForInitializerPad
  • EmptyForIteratorPad
  • EmptyStatement
  • EqualsAvoidNull
  • EqualsHashCode
  • ExecutableStatementCount
  • ExplicitInitialization
  • FallThrough
  • FinalLocalVariable
  • FinalParameters
  • GenericWhitespace
  • HiddenField
  • HideUtilityClassConstructor
  • IllegalInstantiation
  • IllegalToken
  • IllegalTokenText
  • IllegalType
  • InnerAssignment
  • InnerTypeLast
  • InterfaceTypeParameterName
  • JavadocVariable
  • JavaNCSS
  • IllegalImport
  • IllegalThrows
  • ImportControl
  • LeftCurly
  • LocalFinalVariableName
  • LocalVariableName
  • MagicNumber
  • MemberName
  • MethodLength
  • MethodName
  • MethodParamPad
  • MethodTypeParameterName
  • ModifiedControlVariable
  • ModifierOrder
  • MultipleStringLiterals
  • MultipleVariableDeclarations
  • MutableException
  • NestedForDepth
  • NestedIfDepth
  • NestedTryDepth
  • NoWhitespaceAfter
  • NoWhitespaceBefore
  • NPathComplexity
  • OneStatementPerLine
  • OperatorWrap
  • OuterTypeNumber
  • PackageName
  • ParameterAssignment
  • ParameterName
  • ParameterNumber
  • ParenPad
  • RedundantImport
  • RedundantModifier
  • RequireThis
  • ReturnCount
  • RightCurly
  • SeparatorWrap
  • SimplifyBooleanExpression
  • SimplifyBooleanReturn
  • SingleSpaceSeparator
  • StaticVariableName
  • StringLiteralEquality
  • SuperClone
  • SuperFinalize
  • SuppressWarnings
  • ThrowsCount
  • TypecastParenPad
  • TypeName
  • UnnecessarySemicolonInEnumeration
  • UnnecessarySemicolonInTryWithResources
  • UnusedImports
  • UpperEll
  • VisibilityModifier
  • WhitespaceAfter
  • WhitespaceAround

Are we still good about it? Personally I'm happy to contribute, it's a good opportunity to learn different rules.

romani added a commit that referenced this issue Aug 15, 2019
@romani

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

Thank you very much !!
We appreciate your help.

it would be awesome is you create UT with list like - https://github.com/checkstyle/checkstyle/pull/6969/files#diff-67a0806621fe751520ab842060ba14fcL106 , see whole change of #6969 it show how @pbludov improve step by step , but it is very enforced validation on each change, new Checks already come in better form in first commit.
so list of what is not done will be self controlled by UTs and nobody will forget to add such tests during new Check is created. We recently missed such tests creation ... .
We can sync manually progress in your comment, as additional option.

@mincong-h

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

Thank you very much !!
We appreciate your help.

You are welcome. I'm very glad to be here, because Checkstyle helps me a lot in my daily job.

it would be awesome is you create UT with list like - https://github.com/checkstyle/checkstyle/pull/6969/files#diff-67a0806621fe751520ab842060ba14fcL106 , see whole change of #6969 it show how @pbludov improve step by step , but it is very enforced validation on each change, new Checks already come in better form in first commit.
so list of what is not done will be self controlled by UTs and nobody will forget to add such tests during new Check is created. We recently missed such tests creation ... .

Thanks for the hint. I will create it.

We can sync manually progress in your comment, as additional option.

Automation is better 😄

rnveach added a commit that referenced this issue Aug 15, 2019
rnveach added a commit that referenced this issue Aug 15, 2019
rnveach added a commit that referenced this issue Aug 16, 2019
mincong-h added a commit to mincong-h/checkstyle that referenced this issue Aug 16, 2019
mincong-h added a commit to mincong-h/checkstyle that referenced this issue Aug 16, 2019
mincong-h added a commit to mincong-h/checkstyle that referenced this issue Aug 16, 2019
mincong-h added a commit to mincong-h/checkstyle that referenced this issue Aug 16, 2019
mincong-h added a commit to mincong-h/checkstyle that referenced this issue Aug 17, 2019
mincong-h added a commit to mincong-h/checkstyle that referenced this issue Aug 17, 2019
mincong-h added a commit to mincong-h/checkstyle that referenced this issue Aug 18, 2019
mincong-h added a commit to mincong-h/checkstyle that referenced this issue Aug 18, 2019
mincong-h added a commit to mincong-h/checkstyle that referenced this issue Aug 18, 2019
@romani

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

It would be good to name all input files for xpath validation with prefix "Input".
out ITs inputs are named with such prefix.
Even it is meaningful to skip common prefix in specific folder, but it is beneficial to have such prefix is contributor is searching for file in IDE or github (search through whole list of files). With prefix you always know what it is, wihout opening it. As such files are not used in code, length of class name is not an issue.
We will benefit more from reasonable name.

What we have now:
https://github.com/checkstyle/checkstyle/tree/master/src/it/resources/com/google/checkstyle/test/chapter2filebasic/rule21filename
Example: InputOuterTypeFilename1.java
so pattern is Input{Checkname}xxxxxxx.java
the same for sun ITs - https://github.com/checkstyle/checkstyle/tree/master/src/it/resources/com/sun/checkstyle/test/chapter5comments/rule52documentationcomments

But I think we were over caution with name prefix to put SuppressionXpathRegression as prefix. Example: SuppressionXpathRegressionAbstractClassNameInner.java
I am ok even with InputAbstractClassNameInner.java so pattern will be the same as in other ITs, UTs. Because such files are not any different from others, eventually all our Inputs will have "//warn" markers inlined. (I am voting for this).

If above looks a bit brave: What if we name it as InputXpathAbstractClassNameInner.java ? to put Xpath as distinguishing marker from inputs of UT or ITs, but in same time make it a bit short.

@rnveach , @pbludov , @strkkk , @mincong-h , please share your thoughts on this, or vote for proposal.

@mincong-h

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

+1 on Input{Checkname}xxxxxxx.java. It's the simplest naming we can find. Combined with the directory name, the purpose of each input is concise and explicit. Also, these input files are only for internal usage in tests, so as far as contributors can understand them and there's no ambiguity, it's fine IMHO.

rnveach added a commit that referenced this issue Aug 19, 2019
@rnveach

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

Input{Checkname}xxxxxxx.java

Unless we name files as InputSuppression{CheckName}xxxxxx.java, input file names in the IT tier may conflict with similar files in the Test tier.

@rnveach rnveach added this to the 8.24 milestone Aug 19, 2019

@romani

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

input file names in the IT tier may conflict with similar files in the Test tier.

Package name will be different, no compile issues, ... . What kind of conflict in name do you see ?

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

Just the same name. In eclipse, you can open a file by Ctrl + Shift + R. It just leads to easier confusion if 2 files have the same name. IMO, file names should be unique in a project.
I thought we saw this with IT inputs and Test inputs. This is why Test inputs are "Input{CheckName}CheckXxxx.java" and IT inputs are "Input{CheckName}Xxxx.java" where checkname doesn't include "Check" on the end.

mincong-h added a commit to mincong-h/checkstyle that referenced this issue Aug 20, 2019
mincong-h added a commit to mincong-h/checkstyle that referenced this issue Aug 20, 2019
@romani

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

This is why Test inputs are "Input{CheckName}CheckXxxx.java" and IT inputs are "Input{CheckName}Xxxx.java"

I do not think it is intentional, it is far from obvious, you have to know project well to get it. I was not aware of such difference.
ITs are: Google inputs, sun inputs, xpath inputs.
Do we really need to differentiate them with some word after "Input" ?

mincong-h added a commit to mincong-h/checkstyle that referenced this issue Aug 20, 2019
@rnveach

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

@romani I am pretty sure it was intentional as I was the one who did all the file renaming in the beginning. #2161 and #2540.
I couldn't find any specifics written when doing a quick search.
If we don't want to differentiate between each IT section for now, then that is ok. My thought right now was we should separate atleast Checkstyle IT from the rest of the IT. The separation between IT and Test should atleast remain.

@romani

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

Ok.
So you are ok to have files named as "Input{CheckName}CheckXxxx.java" ? To follow name pattern of test IT inputs .

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

The separation between IT and Test should atleast remain.
So you are ok to have files named as "Input{CheckName}CheckXxxx.java"

If we are talking about in the IT tier, I ask that we use a different pattern then Test.
Input{CheckName}Xxxx.java (no check) at the very least since that is the current IT pattern.

@romani

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

Sorry, copied wrong pattern.
Ok, IT pattern should be Input{CheckName}Xxxx.java.

I recommend to add such pattern in validation and allow two patterns (with input prefix and existing ) while work in progress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.