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

Luolc · 2017-03-09T14:01:19Z

The fix will follow the steps below:

Create an input file and let the CI fail with a stackoverflow error.
Rewrite the test with a non-recursive way and let the CI pass. To prove the fix will work.
Create diff report.

~~Currently I am doing step one. I have duplicated the input file which makes CI failed in #3700. I am waiting for the CI result now.~~

The stackoverflow is reproduced successfully. https://travis-ci.org/checkstyle/checkstyle/jobs/209359675#L2123

CI passed after rewriting the test now.

Diff report:
http://www.luolc.com/checkstyle-diff-report/issue3961/

@rnveach please have a review 😄

codecov-io · 2017-03-09T14:45:36Z

Codecov Report

Merging #3966 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #3966   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         283     283           
  Lines       14758   14758           
  Branches     3367    3367           
======================================
  Hits        14758   14758

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 069bf8e...b7ef7bf. Read the comment docs.

rnveach · 2017-03-10T02:32:02Z

src/test/java/com/puppycrawl/tools/checkstyle/api/DetailASTTest.java

+    private static void checkTree(final String filename, final DetailAST root) {
+        DetailAST curNode = root;
+        while (curNode != null) {
+            checkNode(curNode, curNode.getParent(), curNode.getPreviousSibling(), filename, root);


Information sent to the method can't be done using the DetailAST methods.
The whole point of this test is to verify those exact methods are functioning properly.

Example: Parameter prev is set to curNode.getPreviousSibling() and inside the method, it is then verified by assertEquals(badPrevMsg, prev, node.getPreviousSibling());.
All it is doing now is testing the method against itself. If you change getPreviousSibling() to return an invalid value, test would still pass.

Original test kept track of the parent and previous itself. This is why.

Just some information:
This test assumes getFirstChild and getNextSibling are working correctly. We can assume getParent and getPreviousSibling are valid after this check method call.

Done. I misunderstood the purpose of that test before. Sorry for my careless 😅

rnveach

I am now having second thoughts about keeping the bad input file once I saw it is almost 4 megs in size.

@romani what do you think? Should we make an exception and just remove it?

romani · 2017-03-11T00:43:22Z

uhh, and all that only to keep UT code correct.

can we generate such file in runtime in temp folder ?

MEZk · 2017-03-11T09:48:04Z

@romani

and all that only to keep UT code correct.

Agree. Maybe it is better to skip that one file or just generate it in runtime. On the other hand, it can be placed at resources-noncompilable folder with an explanation why it is there.

romani · 2017-03-11T13:57:51Z

On the other hand, it can be placed at resources-noncompilable folder with an explanation why it is there.

it will keep the same size in repo, it very undesirable. Fact that it is NOT a Input for main code lowers its value.

romani

we need to try generate file in run time. Comment over generation code is required to explain.

Luolc · 2017-03-13T16:05:03Z

Sorry for the late reply. I went climbing at the last weekend. 😅

I am now having second thoughts about keeping the bad input file once I saw it is almost 4 megs in size.

@rnveach I didn't simplify the input file before. The original one is just a duplicate of https://github.com/checkstyle/checkstyle/blob/master/src/test/resources/com/puppycrawl/tools/checkstyle/checks/InputAllEscapedUnicodeCharacters.java. It is 12kb after the optimization, and can also be used to reproduce the stackoverflow error.

@romani @MEZk
Since it has a much less size now, is it acceptable? If not, I will try to generate it in run time. But I am not very sure how to do that currently, maybe I need some help later.

BTW, I don't know why wercker/build failed. My other PR failed on it as well and it passed when I had a second try. But this one keeps failing. I could only see a 401 when clicking 'Details' so I am not able to know what's happened.

rnveach · 2017-03-13T17:34:43Z

I am fine with 12kb.

wrecker failure is timeout on checkstyle regression.

romani · 2017-03-13T20:25:27Z

Changes are ok, but we need to pass wercker.

Wercker logs: https://gist.github.com/romani/306d36ba1a6590658db911906b83ac66
Wercker commands: https://github.com/checkstyle/checkstyle/blob/master/wercker.yml#L172
please run wercker commands on local to reproduce the problem, share the ouput.

… tree

Luolc · 2017-03-14T10:59:02Z

This time is codecov/project failing... so weird.
Wercker commands on my local ran successfully. I just push again and it passed... Don't know why.
The details of codecov/project says that This issue occurs when the base commit (the parent commit of the first commit on the pull request) did not upload coverage and/or failed CI tests. It seems that it is a problem of the current head of master. Need help.

MEZk · 2017-03-14T19:04:48Z

@romani
As all CI validations passed and we all agree with the changes I think we are ready to merge PR.

romani

I am Ok with Changes,

Luolc force-pushed the issue3961 branch from ae5aea1 to b4bef41 Compare March 9, 2017 14:45

rnveach requested changes Mar 10, 2017

View reviewed changes

Luolc mentioned this pull request Mar 10, 2017

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

Closed

Luolc force-pushed the issue3961 branch from b4bef41 to 2da268d Compare March 10, 2017 05:30

rnveach approved these changes Mar 10, 2017

View reviewed changes

rnveach requested a review from romani March 10, 2017 16:58

romani requested changes Mar 11, 2017

View reviewed changes

Luolc force-pushed the issue3961 branch 3 times, most recently from 653d9a1 to f7ec6e3 Compare March 13, 2017 15:17

romani approved these changes Mar 13, 2017

View reviewed changes

Luolc force-pushed the issue3961 branch 3 times, most recently from 43cef0f to a47d2fd Compare March 14, 2017 09:29

Issue checkstyle#3961: DetailASTTest: 'checkTree' failing on deep AST…

b7ef7bf

… tree

Luolc force-pushed the issue3961 branch from a47d2fd to b7ef7bf Compare March 14, 2017 10:24

romani approved these changes Mar 14, 2017

View reviewed changes

romani merged commit cb44574 into checkstyle:master Mar 14, 2017

Luolc deleted the issue3961 branch March 15, 2017 07:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Luolc commented Mar 9, 2017 •

edited

Loading

codecov-io commented Mar 9, 2017 •

edited

Loading

rnveach Mar 10, 2017 •

edited

Loading

rnveach Mar 10, 2017 •

edited

Loading

Luolc Mar 10, 2017

rnveach left a comment

romani commented Mar 11, 2017

MEZk commented Mar 11, 2017 •

edited

Loading

romani commented Mar 11, 2017

romani left a comment

Luolc commented Mar 13, 2017 •

edited

Loading

rnveach commented Mar 13, 2017 •

edited

Loading

romani commented Mar 13, 2017

Luolc commented Mar 14, 2017

MEZk commented Mar 14, 2017

romani left a comment •

edited

Loading

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

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

Conversation

Luolc commented Mar 9, 2017 • edited Loading

codecov-io commented Mar 9, 2017 • edited Loading

Codecov Report

rnveach Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

rnveach Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Luolc Mar 10, 2017

Choose a reason for hiding this comment

rnveach left a comment

Choose a reason for hiding this comment

romani commented Mar 11, 2017

MEZk commented Mar 11, 2017 • edited Loading

romani commented Mar 11, 2017

romani left a comment

Choose a reason for hiding this comment

Luolc commented Mar 13, 2017 • edited Loading

rnveach commented Mar 13, 2017 • edited Loading

romani commented Mar 13, 2017

Luolc commented Mar 14, 2017

MEZk commented Mar 14, 2017

romani left a comment • edited Loading

Choose a reason for hiding this comment

Luolc commented Mar 9, 2017 •

edited

Loading

codecov-io commented Mar 9, 2017 •

edited

Loading

rnveach Mar 10, 2017 •

edited

Loading

rnveach Mar 10, 2017 •

edited

Loading

MEZk commented Mar 11, 2017 •

edited

Loading

Luolc commented Mar 13, 2017 •

edited

Loading

rnveach commented Mar 13, 2017 •

edited

Loading

romani left a comment •

edited

Loading