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

Checkstyle UTs shouldn't create custom configuration methods #5157

Closed
rnveach opened this Issue Sep 28, 2017 · 4 comments

Comments

2 participants
@rnveach
Member

rnveach commented Sep 28, 2017

Identified at checkstyle/regression-tool#85 (comment) ,

In our UTs we should use standard configuration methods that are defined in our abstract support classes.
Regression tools needs consistency in order to be able to pull information from the UTs and grab property values.

Example:
FileTabCharacterCheck: createConfig

private static DefaultConfiguration createConfig(boolean verbose) {

We need to find and review these cases and see if they can be replaced with our normal configuration methods or if we need to extend abstract support for more complex cases.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 9, 2017

Member

I doubt that it was done on very good reason, it might be just optimization of code amount, we could sacrifice it in favor of regression-tool.

Member

romani commented Oct 9, 2017

I doubt that it was done on very good reason, it might be just optimization of code amount, we could sacrifice it in favor of regression-tool.

@romani romani added the approved label Oct 9, 2017

@rnveach rnveach added the GSoC2017 label Oct 30, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 16, 2017

Member

@romani Not only are custom createConfig methods an issue but also custom verify methods.
We are looking for the verify method to know we are dealing with a regression area, but these custom methods are named differently like verifySuppressed and verifyWarns.
Example:

private void verifySuppressed(Configuration aFilterConfig,
String... aSuppressed) throws Exception {
verify(createChecker(aFilterConfig),
getPath("InputSuppressWarningsFilter.java"),
removeSuppressed(ALL_MESSAGES, aSuppressed));
}

and
private void verifyWarns(Configuration config, String filePath,
String... expected)
throws Exception {
final int tabWidth = Integer.parseInt(config.getAttribute("tabWidth"));
final IndentComment[] linesWithWarn =
getLinesWithWarnAndCheckComments(filePath, tabWidth);
verify(config, filePath, expected, linesWithWarn);
assertEquals("Expected warning count in UT does not match warn"
+ " comment count in input file", linesWithWarn.length,
expected.length);

How should we handle these areas?

Edit:
We can't go by the start of the method name being verify as there are false positives.
Example:

private void verifyLines(String... expectedLines)

Edit:
My current idea is to make this a customizable option to specify the full method name.

Member

rnveach commented Nov 16, 2017

@romani Not only are custom createConfig methods an issue but also custom verify methods.
We are looking for the verify method to know we are dealing with a regression area, but these custom methods are named differently like verifySuppressed and verifyWarns.
Example:

private void verifySuppressed(Configuration aFilterConfig,
String... aSuppressed) throws Exception {
verify(createChecker(aFilterConfig),
getPath("InputSuppressWarningsFilter.java"),
removeSuppressed(ALL_MESSAGES, aSuppressed));
}

and
private void verifyWarns(Configuration config, String filePath,
String... expected)
throws Exception {
final int tabWidth = Integer.parseInt(config.getAttribute("tabWidth"));
final IndentComment[] linesWithWarn =
getLinesWithWarnAndCheckComments(filePath, tabWidth);
verify(config, filePath, expected, linesWithWarn);
assertEquals("Expected warning count in UT does not match warn"
+ " comment count in input file", linesWithWarn.length,
expected.length);

How should we handle these areas?

Edit:
We can't go by the start of the method name being verify as there are false positives.
Example:

private void verifyLines(String... expectedLines)

Edit:
My current idea is to make this a customizable option to specify the full method name.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 18, 2017

Member

do whatever is convenient for you, I am ok to naming refactoring in test.

Member

romani commented Nov 18, 2017

do whatever is convenient for you, I am ok to naming refactoring in test.

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 19, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 19, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 19, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 23, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 23, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 23, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 23, 2017

romani added a commit that referenced this issue Nov 24, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 24, 2017

Member

Fix was merged

Member

rnveach commented Nov 24, 2017

Fix was merged

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