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

DetailASTTest: 'checkTree' failing on deep AST tree #3961

Closed
rnveach opened this Issue Mar 8, 2017 · 9 comments

Comments

Projects
None yet
3 participants
@rnveach
Member

rnveach commented Mar 8, 2017

Taken from #3894 (comment) and seen at https://travis-ci.org/checkstyle/checkstyle/jobs/208912195#L254 ,

java.lang.StackOverflowError
at com.puppycrawl.tools.checkstyle.api.DetailASTTest.checkTree(DetailASTTest.java:158)

We should find a way to re-write this test, if possible, so it doesn't use the stack for each child.
When we run this on a deeply nested tree, we get a stackoverflow.
We should be able to walk through the tree like we do in other areas, see

private void processIter(DetailAST root, AstState astState) {
DetailAST curNode = root;
while (curNode != null) {
notifyVisit(curNode, astState);
DetailAST toVisit = curNode.getFirstChild();
while (curNode != null && toVisit == null) {
notifyLeave(curNode, astState);
toVisit = curNode.getNextSibling();
if (toVisit == null) {
curNode = curNode.getParent();
}
}
curNode = toVisit;
}

Any file skips in this test should be removed as proof the fix works.

@rnveach rnveach added the easy label Mar 8, 2017

@Luolc

This comment has been minimized.

Show comment
Hide comment
@Luolc

Luolc Mar 9, 2017

Contributor

I have reviewed some codes in DetailASTTest when handling #3700. I think I am able to rewrite the test 😄
I will work on it once approved.

Contributor

Luolc commented Mar 9, 2017

I have reviewed some codes in DetailASTTest when handling #3700. I think I am able to rewrite the test 😄
I will work on it once approved.

@romani romani added the approved label Mar 9, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 9, 2017

Member

Approved

Member

romani commented Mar 9, 2017

Approved

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Mar 9, 2017

Member

@Luolc Since your PR fixed the file with the original test, it is probably best to start off with another file that will fail and then provide fix to show it isn't an issue anymore.

Member

rnveach commented Mar 9, 2017

@Luolc Since your PR fixed the file with the original test, it is probably best to start off with another file that will fail and then provide fix to show it isn't an issue anymore.

@Luolc

This comment has been minimized.

Show comment
Hide comment
@Luolc

Luolc Mar 9, 2017

Contributor

@rnveach That makes sense. So the steps tend to be:

  1. Create an input file and let the CI fail.
  2. Rewrite the test to prove the fix will work on a deeply nested tree.
  3. Delete the input file.
    I will do that step by step 😄
Contributor

Luolc commented Mar 9, 2017

@rnveach That makes sense. So the steps tend to be:

  1. Create an input file and let the CI fail.
  2. Rewrite the test to prove the fix will work on a deeply nested tree.
  3. Delete the input file.
    I will do that step by step 😄
@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Mar 9, 2017

Member

So the steps tend to be
3. Delete the input file.

Yes to all except step 3. The file needs to stay to prove we don't revert back and break the test again.
CI is our mini-regression. We want to keep moving forward and not have to redo fixes again and again.

Member

rnveach commented Mar 9, 2017

So the steps tend to be
3. Delete the input file.

Yes to all except step 3. The file needs to stay to prove we don't revert back and break the test again.
CI is our mini-regression. We want to keep moving forward and not have to redo fixes again and again.

@Luolc

This comment has been minimized.

Show comment
Hide comment
@Luolc

Luolc Mar 9, 2017

Contributor

@rnveach Got it. Just not sure where to put the input file. I am doing with step one now and the input file is put at https://github.com/checkstyle/checkstyle/tree/master/src/test/resources/com/puppycrawl/tools/checkstyle/checks (where the InputAllEscapedUnicodeCharacters in #3700 is). Should I put the file at https://github.com/checkstyle/checkstyle/tree/master/src/test/resources/com/puppycrawl/tools/checkstyle/api? (same package as DetailASTTest)

Contributor

Luolc commented Mar 9, 2017

@rnveach Got it. Just not sure where to put the input file. I am doing with step one now and the input file is put at https://github.com/checkstyle/checkstyle/tree/master/src/test/resources/com/puppycrawl/tools/checkstyle/checks (where the InputAllEscapedUnicodeCharacters in #3700 is). Should I put the file at https://github.com/checkstyle/checkstyle/tree/master/src/test/resources/com/puppycrawl/tools/checkstyle/api? (same package as DetailASTTest)

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 9, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Mar 10, 2017

Member

@Luolc Since its an input file to the test, it should be in the same package as the test.

Member

rnveach commented Mar 10, 2017

@Luolc Since its an input file to the test, it should be in the same package as the test.

@Luolc

This comment has been minimized.

Show comment
Hide comment
@Luolc

Luolc Mar 10, 2017

Contributor

@rnveach Done. Shall we move the discussion to the PR page #3966 ? 😄

Contributor

Luolc commented Mar 10, 2017

@rnveach Done. Shall we move the discussion to the PR page #3966 ? 😄

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 10, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 13, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 13, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 13, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 14, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 14, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 14, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 14, 2017

romani added a commit that referenced this issue Mar 14, 2017

@romani romani added this to the 7.7 milestone Mar 14, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 14, 2017

Member

fix is merged.

Member

romani commented Mar 14, 2017

fix is merged.

@romani romani closed this Mar 14, 2017

SergeyDzyuba added a commit to SergeyDzyuba/checkstyle that referenced this issue Mar 14, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 18, 2017

GitToasterhub added a commit to GitToasterhub/checkstyle that referenced this issue Mar 18, 2017

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