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 #6207: Add xpath regression test for AbstractClassName #6977

Merged

Conversation

mincong-h
Copy link
Contributor

This PR adds XPath regression test for suppression filter on AbstractClassName check, see #6207.

Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

minor item to improve:

@@ -0,0 +1,4 @@
package org.checkstyle.suppressionxpathfilter.abstractclassname;

public abstract class SuppressionXpathRegressionAbstractClassNameOne { // warn
Copy link
Member

@romani romani Aug 14, 2019

Choose a reason for hiding this comment

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

SuppressionXpathRegressionAbstractClassNameOne

it is better to make some reasonable names if possible.
For example:
SuppressionXpathRegressionAbstractClassNameTop
SuppressionXpathRegressionAbstractClassNameInner
SuppressionXpathRegressionAbstractClassNameNoModifier

we did bad naming of "...One/....Two" only when we have no idea how to name them, or name will be too long.

test methods should be named in same patterns:
testModifierOne => testNoModifier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, let me change them.

Copy link
Member

Choose a reason for hiding this comment

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

@romani Correct me if I am wrong, but all input files should start with Input.
It looks like our test to catch this isn't looking at the IT tier.

Copy link
Member

Choose a reason for hiding this comment

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

lets proceed as we have now, after reply of @timurt , I will make a decision.

@mincong-h mincong-h force-pushed the issue-6207-AbstractClassNameCheck branch from 63d0a11 to 6db83d5 Compare August 14, 2019 20:19
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

ok to merge

@rnveach rnveach merged commit 44d578c into checkstyle:master Aug 15, 2019
@mincong-h mincong-h deleted the issue-6207-AbstractClassNameCheck branch August 15, 2019 10:00
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.

None yet

3 participants