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

Pull #14518: move AllBlockCommentTest and AllSinglelineCommentsTest to AST Tests #14518

Merged
merged 1 commit into from Feb 28, 2024

Conversation

mahfouz72
Copy link
Contributor

issue #14249:

  • updated inner test check to do violation on block comments rather than only static update
  • used input file with inline config to test the check

@mahfouz72
Copy link
Contributor Author

I think CI failure is unrelated

Copy link
Member

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

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

Items:

@@ -48,14 +44,15 @@ protected String getPackageLocation() {

@Test
public void testAllBlockComments() throws Exception {
Copy link
Member

@nrmancuso nrmancuso Feb 22, 2024

Choose a reason for hiding this comment

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

Please investigate a bit and leave a comment above this test method (or class) that describes why we need such a strange test. If it is not obvious, let's remove it completely and see what fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nrmancuso Honestly idk I found it weird too. just updated it to be an example for the issue. but I will investigate

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@nrmancuso nrmancuso Feb 24, 2024

Choose a reason for hiding this comment

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

@mahfouz72 if CI is green in #14537, please just do as I have there and delete this class completely. Make sure to grep for this class name to remove any mentions of it as well.

Edit: CI is green, let's proceed with removal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nrmancuso should I close this PR? or just update it after the removal

Copy link
Member

Choose a reason for hiding this comment

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

Input file that is full of block multi line comments https://github.com/checkstyle/checkstyle/blob/master/src/test/resources/com/puppycrawl/tools/checkstyle/grammar/comments/InputFullOfBlockComments.java

We just need to change

/*0*//*1*/package/*2*/ com/*3*/./*4*/puppycrawl/*5*/./*6*/tools/*7*/./*8*/checkstyle.grammar/*9*/./*10*/comments/*11*/;/*12*/
/*13*/

To more verbose:

/*0*//*1*/package/*2*/ com/*3*/./*4*/puppycrawl/*5*/./*6*/tools/*7*/./*8*/checkstyle.grammar/*9*/./*10*/comments/*11*/;/*12*/
// 12 violations above:
//.           '0'
//.           '1'

Copy link
Member

Choose a reason for hiding this comment

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

This test just test our ability to detect comments at any possible place. It might not be required for coverage but it helps to control parsing

Ok, then let’s create a test for the AST with comments generated by this file.

Copy link
Member

Choose a reason for hiding this comment

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

yes, good idea, this is exactly what we wanted many years ago, but we did not have this concept at all.

Copy link
Member

Choose a reason for hiding this comment

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

@mahfouz72 , please move this test to AST tests
Example:

public void testAdvanceJava9TryWithResourcesAstTree() throws Exception {
verifyAst(getPath("ExpectedAdvanceJava9TryWithResources.txt"),
getPath("/java9/InputAdvanceJava9TryWithResources.java"));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@romani done I have moved singlelineCommentTest also to AST test. CI will be green after merging #14567

@nrmancuso nrmancuso self-assigned this Feb 22, 2024
@mahfouz72 mahfouz72 force-pushed the issue-14249 branch 2 times, most recently from 2105173 to f077d26 Compare February 22, 2024 11:19
mahfouz72 added a commit to mahfouz72/checkstyle that referenced this pull request Feb 25, 2024
@mahfouz72 mahfouz72 changed the title Issue #14249: update inner test Check in AllBlockCommentTest to do vi… Pull #14518: Remove AllBlockCommentTest Feb 25, 2024
Copy link
Member

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

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

Glad to clean things up, @mahfouz72 thanks a lot for this!

@nrmancuso nrmancuso assigned romani and unassigned nrmancuso Feb 25, 2024
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.

Please see comment above

mahfouz72 added a commit to mahfouz72/checkstyle that referenced this pull request Feb 26, 2024
mahfouz72 added a commit to mahfouz72/checkstyle that referenced this pull request Feb 26, 2024
mahfouz72 added a commit to mahfouz72/checkstyle that referenced this pull request Feb 26, 2024
@mahfouz72
Copy link
Contributor Author

Please see comment above

@romani please review

mahfouz72 added a commit to mahfouz72/checkstyle that referenced this pull request Feb 28, 2024
@mahfouz72 mahfouz72 changed the title Pull #14518: Remove AllBlockCommentTest Pull #14518: move AllBlockCommentTest and AllSinglelineCommentsTest to AST Tests Feb 28, 2024
mahfouz72 added a commit to mahfouz72/checkstyle that referenced this pull request Feb 28, 2024
Copy link
Member

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

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

A lot of back and forth on this one, @mahfouz72 thanks for hanging in there!

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.

I like this update, it becomes much better

@romani romani merged commit a801b1e into checkstyle:master Feb 28, 2024
111 of 113 checks passed
@github-actions github-actions bot added this to the 10.14.1 milestone Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants