-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 #11446: Update SuppressionCommentFilterTest to use verifyWithInlineConfigParser #11464
Issue #11446: Update SuppressionCommentFilterTest to use verifyWithInlineConfigParser #11464
Conversation
GitHub, rebase |
1 similar comment
GitHub, rebase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Items
* @throws Exception if there is a problem during checker configuration | ||
*/ | ||
protected final void execute(Configuration moduleConfig, String fileName, | ||
String[] expectedViolations, String... suppressedViolations) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not need these parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we have the verify
method in verifySuppressed
which uses these 2 parameters like expected violation
and suppressed violations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e77a62f#diff-cf838e5bbdb7de5a3e0e91f9919a389a0fd2ffdbda60c85a3c6dba07dcf69acdR406-R420 only deals with config and file name and when I am using this execute method then we can't use expected and suppressed violation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private void verifySuppressed(Configuration moduleConfig, String fileName)
throws Exception {
execute(moduleConfig, getPath(fileName));
}
Like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New method should be created.
Please do not push your code until mvn verify
is passing on local.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need Help:
- I started with creating:
private void verifySuppressed(Configuration moduleConfig, String fileName)
throws Exception {
execute(moduleConfig, getPath(fileName));
}
in the SuppressionCommentFilterTest.java
- I saw that I should change this
verifySuppressed
method of 3 parameters with myverifySuppressed
method that only takes 2 parameters.
@Test
public void testInvalidMessageFormat() throws Exception {
final DefaultConfiguration filterConfig =
createModuleConfig(SuppressionCommentFilter.class);
filterConfig.addProperty("messageFormat", "e[l");
try {
final String[] suppressed = CommonUtil.EMPTY_STRING_ARRAY;
verifySuppressed(filterConfig, "InputSuppressionCommentFilter11.java", suppressed);
assertWithMessage("Exception is expected").fail();
}
catch (CheckstyleException ex) {
final IllegalArgumentException cause = (IllegalArgumentException) ex.getCause();
assertWithMessage("Invalid exception message")
.that(cause)
.hasMessageThat()
.isEqualTo("unable to parse expanded comment e[l");
}
}
Like this -->
try {
verifySuppressed(filterConfig, "InputSuppressionCommentFilter11.java");
assertWithMessage("Exception is expected").fail();
}
But now I am getting an error of EXCEPTION IS EXPECTED
PS; I haven't changed the verify() method in verifySuppressed method
which takes 4 parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you updating too much.
you need to remove methods:
- private void verifySuppressed(Configuration moduleConfig, String fileName,
- String... aSuppressed)
- throws Exception {
-
- private void verifySuppressed(Configuration moduleConfig, String fileName,
- String[] expectedViolations, String... suppressedViolations) throws Exception {
and figure out how to upadate test that that used them to execute validation by checkstyle but wihtout comparison with expected violation, as you expect exception.
test has problem with path to file and Input file is over complicated for no reason, so please redo Input file content and have only config for filter and for one Check and in code to have only one violation (no more code in Input file is requied).
time to debug how Checks are working :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Items
@@ -406,8 +396,7 @@ public void testInvalidMessageFormat() throws Exception { | |||
filterConfig.addProperty("messageFormat", "e[l"); | |||
|
|||
try { | |||
final String[] suppressed = CommonUtil.EMPTY_STRING_ARRAY; | |||
verifySuppressed(filterConfig, "InputSuppressionCommentFilter11.java", suppressed); | |||
verifySuppressed(filterConfig, "InputSuppressionCommentFilter11.java"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method should be "execute" as we do not validate anything, we just run and expect exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have used execute method in the verifySuppressed method which takes 2 parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use it inside, use it here.
Follow complains from CI |
@Rahulkhinchi03, ping |
@romani I am currently debugging the code and trying to find the reason for |
…lineConfigParser
Resolves update for SuppressionCommentFilterTest in #11446.