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

Refactor Javadoc AST Tests #4381

Closed
rnveach opened this Issue May 28, 2017 · 15 comments

Comments

@rnveach
Member

rnveach commented May 28, 2017

Identified at #4378 and similar to #2396 ,

We should drop creating custom trees in our tests for Javadocs and compare the output printing the tree, see proposal details at #2396 . We do similar comparisons for our Java grammar in AstRegressionTest with verifyAst.

We should do this conversion for ParseTreeBuilder.

@Vladlis Vladlis added the javadoc label May 28, 2017

@Vladlis

This comment has been minimized.

Show comment
Hide comment
@Vladlis

Vladlis May 28, 2017

Member

#3796 should be done first to avoid double refactoring

Member

Vladlis commented May 28, 2017

#3796 should be done first to avoid double refactoring

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 3, 2017

Member

It should be noted that when refactoring, some trees in ParseTreeBuilder are not in use at all.
Example:
https://github.com/checkstyle/checkstyle/search?utf8=%E2%9C%93&q=treeAsteriskInJavadocInlineTag&type=

public static ParseTree treeAsteriskInJavadocInlineTag()

It was traced back to b4c351b but has no input file connected to it in that commit, but may be based on this file: https://github.com/checkstyle/checkstyle/blob/25a37e5049b7816b34c552899841a978efc37a63/src/test/resources/com/puppycrawl/tools/checkstyle/grammars/javadoc/javadocTags/InputAsteriskInJavadocInlineTag.txt

Member

rnveach commented Jun 3, 2017

It should be noted that when refactoring, some trees in ParseTreeBuilder are not in use at all.
Example:
https://github.com/checkstyle/checkstyle/search?utf8=%E2%9C%93&q=treeAsteriskInJavadocInlineTag&type=

public static ParseTree treeAsteriskInJavadocInlineTag()

It was traced back to b4c351b but has no input file connected to it in that commit, but may be based on this file: https://github.com/checkstyle/checkstyle/blob/25a37e5049b7816b34c552899841a978efc37a63/src/test/resources/com/puppycrawl/tools/checkstyle/grammars/javadoc/javadocTags/InputAsteriskInJavadocInlineTag.txt

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Jun 16, 2017

Collaborator

@romani @rnveach @Vladlis
These tests are for the ANTLR parser we use. If we do things the way we do with AstRegressionTest we would be actually verifying the DetailNodeTreeStringPrinter and JavadocDetailNodeParser. We won't be able to verify the ANTLR parser directly.

We sort of parse javadoc twice. Once with ANTLR and then we process the whole resulting tree and convert it to (or generate) a DetailNode tree. Right now the javadoc tests to be refactored are verifying the ANTLR parser and rightfully belong to the JavadocParseTreeTest class. But if we start verifying the final tree that gets generated then those tests should perhaps belong to DetailNodeTreeStringPrinterTest.

I wish that we were able to parse just once and use ANTLR generated tree as it is. I am not sure though if that can be achieved with an efficient design or not. Will have to look into it.

Collaborator

ps-sp commented Jun 16, 2017

@romani @rnveach @Vladlis
These tests are for the ANTLR parser we use. If we do things the way we do with AstRegressionTest we would be actually verifying the DetailNodeTreeStringPrinter and JavadocDetailNodeParser. We won't be able to verify the ANTLR parser directly.

We sort of parse javadoc twice. Once with ANTLR and then we process the whole resulting tree and convert it to (or generate) a DetailNode tree. Right now the javadoc tests to be refactored are verifying the ANTLR parser and rightfully belong to the JavadocParseTreeTest class. But if we start verifying the final tree that gets generated then those tests should perhaps belong to DetailNodeTreeStringPrinterTest.

I wish that we were able to parse just once and use ANTLR generated tree as it is. I am not sure though if that can be achieved with an efficient design or not. Will have to look into it.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 16, 2017

Member

we would be actually verifying the DetailNodeTreeStringPrinter and JavadocDetailNodeParser. We won't be able to verify the ANTLR parser directly.

What would we need to verify from the ANTLR parser directly that we can't achieve, or is lost, through the printers?

No one uses the parser directly. All our modules and such rely on the results that come from JavadocDetailNodeParser. This is the same with Java's TreeWalker and grammar.
The point of these tests is for regression on our grammar, hence the name of AstRegressionTest. Even when we do regression tests on javadoc using the new shell script, we use the -J option which is the final tree, not the actual ANTLR grammar.

Member

rnveach commented Jun 16, 2017

we would be actually verifying the DetailNodeTreeStringPrinter and JavadocDetailNodeParser. We won't be able to verify the ANTLR parser directly.

What would we need to verify from the ANTLR parser directly that we can't achieve, or is lost, through the printers?

No one uses the parser directly. All our modules and such rely on the results that come from JavadocDetailNodeParser. This is the same with Java's TreeWalker and grammar.
The point of these tests is for regression on our grammar, hence the name of AstRegressionTest. Even when we do regression tests on javadoc using the new shell script, we use the -J option which is the final tree, not the actual ANTLR grammar.

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Jun 17, 2017

Collaborator

This is the same with Java's TreeWalker and grammar

But we don't post process the tree we parse for java. We don't have a parser of our own for java. We just call parser.getAst() and don't post process the result with our own parser. TreeWalker is just for walking the tree. We walk the same tree we obtain from ANTLR. For javadoc we are not walking the ANTLR generated tree but the DetailNode tree we generate from the ANTLR generated Tree.

What would we need to verify from the ANTLR parser directly that we can't achieve, or is lost, through the printers?

I can't think of anything straightaway. But they are two different units and the post processing involved is heavy. ANTLR tree is not the one that is printed. Again, we generate a DetailNode tree from an ANTLR tree and then print it. DetailNode is a class that we created and isn't something from ANTLR. So if we change the tests we would be only indirectly testing one unit by directly testing the other. Perhaps it can be logically proven that both indirect and direct testing of ANTLR parser is equivalent, but we don't know that as of yet.

Collaborator

ps-sp commented Jun 17, 2017

This is the same with Java's TreeWalker and grammar

But we don't post process the tree we parse for java. We don't have a parser of our own for java. We just call parser.getAst() and don't post process the result with our own parser. TreeWalker is just for walking the tree. We walk the same tree we obtain from ANTLR. For javadoc we are not walking the ANTLR generated tree but the DetailNode tree we generate from the ANTLR generated Tree.

What would we need to verify from the ANTLR parser directly that we can't achieve, or is lost, through the printers?

I can't think of anything straightaway. But they are two different units and the post processing involved is heavy. ANTLR tree is not the one that is printed. Again, we generate a DetailNode tree from an ANTLR tree and then print it. DetailNode is a class that we created and isn't something from ANTLR. So if we change the tests we would be only indirectly testing one unit by directly testing the other. Perhaps it can be logically proven that both indirect and direct testing of ANTLR parser is equivalent, but we don't know that as of yet.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 17, 2017

Member

we don't post process the tree we parse for java
We walk the same tree we obtain from ANTLR

We do post-processing for the comments.
One reason we don't have to do this is because the Java grammar is filled with custom Java code inside the grammar and inside DetailAST, which is a slightly bad design. It means the grammar can only be used with the Java language. If we ever switch to ANTLR4 for Java, we will probably have to change to heavier post-processing of the nodes created like Javadoc.

I can't think of anything straightaway.
Perhaps it can be logically proven that both ... is equivalent, but we don't know that as of yet

pitest would show us if we are losing important lines covered by tests. Passing pitest with no extra tests would show that the 2 forms of testing are exactly equivalent.
In fact, extra tests that directly and clearly test something special in the JavadocDetailNodeParser/grammar would be better. It would make it clearer some specifics we doing during the extra post-processing, like it does at #4455 (comment) . This can't be seen right now since all the tests are the same.

Member

rnveach commented Jun 17, 2017

we don't post process the tree we parse for java
We walk the same tree we obtain from ANTLR

We do post-processing for the comments.
One reason we don't have to do this is because the Java grammar is filled with custom Java code inside the grammar and inside DetailAST, which is a slightly bad design. It means the grammar can only be used with the Java language. If we ever switch to ANTLR4 for Java, we will probably have to change to heavier post-processing of the nodes created like Javadoc.

I can't think of anything straightaway.
Perhaps it can be logically proven that both ... is equivalent, but we don't know that as of yet

pitest would show us if we are losing important lines covered by tests. Passing pitest with no extra tests would show that the 2 forms of testing are exactly equivalent.
In fact, extra tests that directly and clearly test something special in the JavadocDetailNodeParser/grammar would be better. It would make it clearer some specifics we doing during the extra post-processing, like it does at #4455 (comment) . This can't be seen right now since all the tests are the same.

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Jun 17, 2017

Collaborator

@rnveach fair enough. One more thing, so the tests should remain in JavadocParseTreeTest or should be transferred to DetailNodeTreeStringPrinterTest ? What do you think ?

Collaborator

ps-sp commented Jun 17, 2017

@rnveach fair enough. One more thing, so the tests should remain in JavadocParseTreeTest or should be transferred to DetailNodeTreeStringPrinterTest ? What do you think ?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 17, 2017

Member

so the tests should remain in JavadocParseTreeTest or should be transferred to DetailNodeTreeStringPrinterTest ?

I don't want to move them to DetailNodeTreeStringPrinterTest because these tests are more regression than testing that class. I am fine with them remaining in JavadocParseTreeTest for now unless we want to move them to some new class.

Member

rnveach commented Jun 17, 2017

so the tests should remain in JavadocParseTreeTest or should be transferred to DetailNodeTreeStringPrinterTest ?

I don't want to move them to DetailNodeTreeStringPrinterTest because these tests are more regression than testing that class. I am fine with them remaining in JavadocParseTreeTest for now unless we want to move them to some new class.

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 17, 2017

Issue #4381: Refactored Javadoc AST tests. They now use trees printed…
… by DetailNodeTreeStringPrinter to verify the javadoc grammar

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 17, 2017

Issue #4381: Refactored Javadoc AST tests. They now use trees printed…
… by DetailNodeTreeStringPrinter to verify the javadoc grammar
@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Jun 17, 2017

Collaborator

@rnveach There are 8 files in ...grammars/javadoc which have no UTs related to them. The will lead to added UTs. May be we can add them in a separate "minor: Added javadoc UTs" commit rather than adding them in this issue by splitting into 2 commits ?

Collaborator

ps-sp commented Jun 17, 2017

@rnveach There are 8 files in ...grammars/javadoc which have no UTs related to them. The will lead to added UTs. May be we can add them in a separate "minor: Added javadoc UTs" commit rather than adding them in this issue by splitting into 2 commits ?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 17, 2017

Member

@ps-sp Are they the files I made note of at #4381 (comment) ?
I don't think should be minor. IMO, they should all be part of this issue.
1 or 2 commits doesn't bother me.

Member

rnveach commented Jun 17, 2017

@ps-sp Are they the files I made note of at #4381 (comment) ?
I don't think should be minor. IMO, they should all be part of this issue.
1 or 2 commits doesn't bother me.

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Jun 17, 2017

Collaborator

@rnveach yes they are. Ok, 2 commits then.

Collaborator

ps-sp commented Jun 17, 2017

@rnveach yes they are. Ok, 2 commits then.

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 18, 2017

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Jun 18, 2017

Collaborator

@rnveach we don't want the input javadoc files to be .javadoc instead of .txt. There are .javadoc input files for a few UTs.

Collaborator

ps-sp commented Jun 18, 2017

@rnveach we don't want the input javadoc files to be .javadoc instead of .txt. There are .javadoc input files for a few UTs.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 18, 2017

Member

we don't want the input javadoc files to be .javadoc instead of .txt

I didn't really notice them before. We can put the javadocs into .javadoc and put the resulting trees into .txt.

Member

rnveach commented Jun 18, 2017

we don't want the input javadoc files to be .javadoc instead of .txt

I didn't really notice them before. We can put the javadocs into .javadoc and put the resulting trees into .txt.

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Jun 18, 2017

Collaborator

@rnveach ok. Also there seem to be some mismatches in the Input files that weren't previously in any UTs. For example look at this , this and this. You can have a look at the trees printed for them here

Collaborator

ps-sp commented Jun 18, 2017

@rnveach ok. Also there seem to be some mismatches in the Input files that weren't previously in any UTs. For example look at this , this and this. You can have a look at the trees printed for them here

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 21, 2017

Issue #4381: Refactored Javadoc AST tests. They now use trees printed…
… by DetailNodeTreeStringPrinter to verify the javadoc grammar

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 21, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 21, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 21, 2017

Issue #4381: Refactored Javadoc AST tests. They now use trees printed…
… by DetailNodeTreeStringPrinter to verify the javadoc grammar

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 21, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 21, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 21, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 22, 2017

Issue #4381: Refactored Javadoc AST tests. They now use trees printed…
… by DetailNodeTreeStringPrinter to verify the javadoc grammar

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 22, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 22, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 22, 2017

Issue #4381: Refactored Javadoc AST tests. They now use trees printed…
… by DetailNodeTreeStringPrinter to verify the javadoc grammar

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 22, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 22, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 22, 2017

Issue #4381: Refactored Javadoc AST tests. They now use trees printed…
… by DetailNodeTreeStringPrinter to verify the javadoc grammar

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 22, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 22, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 24, 2017

Issue #4381: Refactored Javadoc AST tests. They now use trees printed…
… by DetailNodeTreeStringPrinter to verify the javadoc grammar

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 24, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 24, 2017

romani added a commit that referenced this issue Jun 25, 2017

Issue #4381: Refactored Javadoc AST tests. They now use trees printed…
… by DetailNodeTreeStringPrinter to verify the javadoc grammar

romani added a commit that referenced this issue Jun 25, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 25, 2017

Member

fix is merged.

Member

romani commented Jun 25, 2017

fix is merged.

@romani romani closed this Jun 25, 2017

@romani romani added this to the 8.0 milestone Jun 25, 2017

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