fixes evaluation stack trace for unit tests with ant#8568
fixes evaluation stack trace for unit tests with ant#8568homberghp wants to merge 10 commits intoapache:masterfrom
Conversation
homberghp
changed the title
homberghp
changed the title
|
solves #8567 |
matthiasblaesing
left a comment
matthiasblaesing
left a comment
There was a problem hiding this comment.
Thank you. The approach makes sense to me. I left a few inline comments. For individual commits: They can be separate, but don't need to be. For the summary of the commit I would suggest:
"JUnit: Fix 'jump to test' for failing unittests"
If I understand it correctly, The stacktrace jumping itself is not affected, it is just the jump, when a failing testcase is selected and a non-stacktrace line is selected and thus no "real" target can be determined.
java/junit.ant.ui/src/org/netbeans/modules/junit/ant/ui/AntJUnitNodeOpener.java
Outdated
Show resolved
Hide resolved
java/junit.ant.ui/src/org/netbeans/modules/junit/ant/ui/AntJUnitNodeOpener.java
Show resolved
Hide resolved
| if (testfo != null && file == null && methodNode.getTestcase().getTrouble() != null && lineNumStorage[0] == -1) { | ||
| //213935 we could not recognize the stack trace line and map it to known file | ||
| //if it's a failure text, grab the testcase's own line from the stack. | ||
| String fqMethodName= methodNode.getTestcase().getClassName()+ '.'+ methodNode.getTestcase().getName(); |
There was a problem hiding this comment.
This does not work (at least in my tests it does not). The name of the testcase also holds the class name, but maybe that is already the problem and we should fix it at the root?!
This is the code I use to fix the nested test cases:
Seems for ant this is not a problem as the report is extracted differently.
java/maven.junit.ui/src/org/netbeans/modules/maven/junit/ui/MavenJUnitNodeOpener.java
Outdated
Show resolved
Hide resolved
java/maven.junit.ui/src/org/netbeans/modules/maven/junit/ui/MavenJUnitNodeOpener.java
Outdated
Show resolved
Hide resolved
matthiasblaesing
added
Java
|
@homberghp This currently fails to build from source. This import looks broken: |
matthiasblaesing
left a comment
matthiasblaesing
left a comment
There was a problem hiding this comment.
Sorry for the late reply. I had a look at this. I think this can be much simpler. The maven change is not necessary (see inline comment).
For the AntJUnitNodeOpener I had a closer look at what is happening.
As in the maven case the stacktrace is checked from the most toplevel method downwards. Index is decremented and this is the stacktrace I'm talking about:
Crucial is the check in the original lines (180-182):
That check is specificly for the "open testcase" case as then the frameinfo is empty. You can verify, that this For me the check in line 181 makes no sense and I think the simplest fix would be to go back to the current state of master and replace line 181 with
methodNodeParentOfStackTraceNode = testfo.equals(file);
This basicly just selected the highest entry in the stacktrace that points into the file that is already determined to be the correct one.
What do you think?
| } | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
I see problems here:
UIJavaUtils.getFilecan yieldnulland while that is a bogus value, it would stop the iteration.- before the change there was a null guard, did you check, that
getStackTraceis always non null?
I also think that this method already does "the right thing". At least my manual tests worked as expected (with this reverted I'm still taken to the testcase, not the deeper assert in the same class/file).
The stacktrace starts with the "deepest" method and ends at the most top level method. It is scanned from last to first index and stops at the first method, for which the file matches the expected file. So it will match the testcase method, not some deeply nested assert.
I remember, that I asked that this should unified, but I think ant and maven drifted away from each other, so lets keep this as it was.
There was a problem hiding this comment.
I am a bit confused now.
Are we in agreement that the search order should be top-to-bottom rather than reverse?
This is what both the ANT and MAVEN variants do wrong, IMHO, and triggered me to post issue #8567.
If UIJavaUtils yields null, then the iteration is correctly stopped. The null value is considered in lines 205 and 206.
There was a problem hiding this comment.
I assume that we talk about the same top and bottom definitions. I define the top frame as the frame that contains the method that "executes as thread"/is the root of the stack trace. This method invokes other methods, and the stack goes down from there. The bottom of the stack trace is thus the last method that was invoked.
Now please have a look at the screenshot of my last comment. What you see there is the st array, the stack trace. If you look closely, you notice, that the order of the lines is so, that the highest index holds the top most frame. st[9] reads at java.base/java.lang.Thread.run(Thrad.java:1583). So if iteration starts at the last index, the stack trace is traversed from top to bottom.
This is exactly what is done (this is current master):
- line 172 initializes index to the last position
- iteration starts
UIJavaUtilsis used to find the file and line the stack trace line points to (line 176)- the next index to check is the previous entry in the stack trace array (line 177) (we are going down the stack)
- ignoring the IMHO bogus lines 180-182, iteration ends when
a. the file determined for the stack trace line matches the file for the test case or
b. the bottom of the stack trace is reached or
c. the check in lines 180-182 is true
5c tests if the file is in the same order as testfo (the testcase file) or in a folder below that.
There was a problem hiding this comment.
Is line 180-182 the cause of the actual issue seen? It will match a sibling of the test file before the test file itself?
Anyway, few old links that might be of use to consider why the code is how it is -
There was a problem hiding this comment.
@matthiasblaesing
I use the top and bottom definitions as implied by Throwable#getStackTrace.
That makes the top the element at index 0. That makes the top the last method call, and the bottom the actual start of the program, in this case, the start of the Junit runner.
@neilcsmith-net, Thanks for the links to the earlier work, though I can't access the bugzilla site.
For the ANT solution:
In my approach, I first iterate through the stack trace to find the fully qualified method name.
If that succeeds, I have the best possible match.
If not, I simply return the top-most stack trace, and try to assign the file with getFile....
I think that the actual order of evaluation matters.
It might very well be the line that triggers the break, which tests for a match with the fully qualified file name.
if (st[index].contains(fqMethodName)) {
file = UIJavaUtils.getFile(st[index], lineNumStorage, locator);
break;
}But that was already in the original solution.
I have seen the effect of considering a sibling; that actually triggered me to raise issue #8567. I have updated the issue description accordingly. I have not yet written an actual (Junit) test, because that would either lead to a very complex test setup or some major refactoring in the XXNodeOpener files, to extract the iteration and its result into separate (testable) methods.
What I observed (and still can observe) in my experiments is that the top of the stack is used with 'goto source', whereas the actual stack line I want to use is a few lines down in the stack trace, so with a higher index.
There was a problem hiding this comment.
I have been trying to reproduce that with a simple test project, but I failed to do so. Both approaches appear to behave similarly.
That may be because the tests in NetBeans refactoring defer the test run, which could cause the effect.
However, when I compare the two approaches, I find that the current NetBeans approach lands me on the method on line 489.
which is the first non-infra stack trace line and is in a sibling of the test class,
whereas the approach I propose lands me inside the test method, at the precise invocation of the verification method on line 64, as it should be IMHO.
The dark theme is the NB 29 installed as binary from netbeans.org; the light theme is my local build on my branch issue8567.
Note: I do not recall noticing that what #8567 describes occurs with anything other than NetBeans internal tests.
There was a problem hiding this comment.
@homberghp you should be able to create an account on the bugzilla instance. Anonymous viewing is disabled. The first issue has a Maven project attached that might be useful to replicate the concern that led to the code being as it is now. The second issue that added the match to sibling files has a lot less information - the commit seems related to inner / nested classes except that part?
It might be worth adding an example stack trace from your issue here (if you haven't already?) and agreeing where it should end up first?
|
Stacktrace In this NetBeans internal test, the 'goto source' action lands me on line |
That makes sense to me too. However, I'm not sure that means the evaluation order is incorrect. Iterating from either end lands you in |
|
It maybe that the evaluation does not matter, but somehow the version going bottom up lands in the wrong location. Going from top to bottom appears logical to me. Maybe my findings are only relevant to NetBeans internal tests. That is annoying but maybe irrelevant for the general case. |
According to that stack trace you shared, it would do that whichever way you iterated the stack. And if If you just comment out the lines that search for the sibling file, does this work correctly in all your test cases? |
|
Indeed, the iteration order is not the problem per se, although starting at the top is closest to the culprit, so the iteration count is lower. That does not really matter, since most stack traces are short in tests, so a big-O analysis does not help in this case. However, the condition to accept a result in the loop does. If it would only accept a stack trace line when it is inside the test method, then that would be perfectly fine. But the expression used is a compound one, with three tests:
Because of this compound condition and the order in which the stacktrace is iterated, it stops at the first matching line, which might be the desired file, or a sibling of the test file. Both terminate the loop. The compoundness (if that is a word) of the expression is the problem. Note that in my proposal, I only consider the fully qualified test method name in the first iteration. Why you would want to look for a sibling is not clear to me. |
|
I restored the 'bottom to top' iteration and changed the condition. |
Me neither, which is why I suggested just removing the lines added in the iteration by codelerity/netbeans-releases@f2a3522 Given this change was about inner / nested classes, I wonder whether it was confusion about |
|
I also undid all changes to the maven variant. |
That might be. Even inner members (classes, interfaces, enums, and records) get their own class file. Anyway, the iteration order is back to the original. And after testing both a 'normal' project and the project that started all this (NetBeans refactoring back in June last year), it works as intended. |
java/junit.ant.ui/src/org/netbeans/modules/junit/ant/ui/AntJUnitNodeOpener.java
Outdated
Show resolved
Hide resolved
|
Applied all improvements as suggested by matthias. |
|
implement remaining suggestion by matthias. |
| index--; | ||
| } | ||
| // if not found, return top line of stack trace. | ||
| if (index == -1) { |
There was a problem hiding this comment.
i believe this could be changed to if (file == null) which is the criteria for success?
On first glance this algorithm looks more complicated than it actually is, I believe the main reason is because the index is passed to the second loop for the lone purpose of reusing the variable.
but its in essence two independent for-each loops, both seem to be in reverse.
we can't do java 21 just yet but soon we might be able to. update: we can now use 21 everywhere. So this could probably be 2x reverse view iterations like
for (String trace : Arrays.asList(st).reversed()) {
if (found) {
file = ...;
break;
}
}or 2x by using java 8 streams
Optional<File> result = Stream.of(st)
.sorted(Comparator.reverseOrder())
.filter(trace -> {
return true; // todo
})
.findFirst();
// if no result do round twomight work too
There was a problem hiding this comment.
Since these stacktrace lines are strings, sorting and reversing them would bring them into serious 'durcheinander'. I am still in doubt that the traces should be visited from bottom to top.
Defensive programming dictates the second loop and final test, but in reality, it is only academic that a stacktrace of a tsst invocation would NOT contain the testmethod.
There was a problem hiding this comment.
you are right the stream example is incorrect since sorting is not what we want. My brain did some wrong name matching there. The first example creates a reverse view which was what I wanted to convey.
I suppose it could be reversed once with Collecitons.reversed(Arrays.asList()) to the same effect on JDK 17. Sorry to cause confusion.
There was a problem hiding this comment.
if ((file == null) && (methodNode.getTestcase().getTrouble() != null) && lineNumStorage[0] == -1) {
String[] st = methodNode.getTestcase().getTrouble().getStackTrace();
if ((st != null) && (st.length > 0)) {
List<String> stack = new ArrayList<>(Arrays.asList(st));
Collections.reverse(stack);
//Jump to the first line matching the fully qualified test method name.
// and ignore the infrastructure stack lines in the process
for (String frame : stack) {
if (frame.contains(fqMethodName)) {
file = UIJavaUtils.getFile(frame, lineNumStorage, locator);
break;
}
}
// if not found, return top line of stack trace.
if (file == null) {
for (String frame : stack) {
String trimmed = JavaRegexpUtils.specialTrim(frame);
if (trimmed.startsWith(JavaRegexpUtils.CALLSTACK_LINE_PREFIX_CATCH)
|| trimmed.startsWith(JavaRegexpUtils.CALLSTACK_LINE_PREFIX)) {
file = UIJavaUtils.getFile(frame, lineNumStorage, locator);
if (testfo.equals(file)) {
break;
}
}
}
}
// if that fails, return the test file object.
if (file == null) {
openTestMethod(methodNode);
return;
}
}
}i think this should work (not tested since I am not at an IDE atm - can be easily changed to list.reversed() once codebase can use 21) update: last issues were resolved and we can now use 21 everywhere
There was a problem hiding this comment.
It might, but I'd still like some confirmation that just commenting out the lines
doesn't fix the issue before we start overcomplicating this. The original code from 213935 still seems correct, and is used elsewhere.There was a problem hiding this comment.
The comment above the second loop now contradicts what actually happens. The reversed list makes the stacktrace go upside down, so the first match that looks like a stack trace line is selected.
I my self keep thinking that the stack traversal from top to bottom (i.e. start at index 0) is the preferred approach. However, that does not happen here. That also fits perfectly to a java 8 stream approach. The findFirst returns an optional which should be insected, with the orElse part should do the parts in the 2nd loop.
If the second loop should do what the comment says, then that should surely start at index 0.
The current code is quite hard to test, unit test wise, because its effect is only observable by side effects in the UI.
| if (testfo.equals(file)) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Sorry, I missed this on the first pass:
| if (testfo.equals(file)) { | |
| break; | |
| } | |
| if (testfo.equals(file)) { | |
| break; | |
| } else { | |
| file = null; | |
| lineNumStorage[0] = -1; | |
| } |
if the match is in the wrong file we need to reset the values so that the fallback in line 196 can correctly be hit.
There was a problem hiding this comment.
Implemented as suggested.
|
The label for maven should be removed. |
matthiasblaesing
left a comment
matthiasblaesing
left a comment
There was a problem hiding this comment.
My final request: Please rebase onto current master and squash all changes into a single commit.
will do, as soon as my grandchildren sleep. We should also change the title, now that the order is still the same. There still is an internal comment that does not fit anymore. line 180. |
|
I'm (still) -0 on this, but won't block it. I still don't see reasoning where the original code, minus the sibling check, is doing something wrong. Still, please ensure this is rebased and squashed before merging if it is. Has it been checked that this behaves correctly with inner / nested classes as per 236056? Unfortunately the test in that issue no longer exists to check against. Might want to consider whether |
neilcsmith-net
added
Ant
|
would be good to simplify this before merge. I haven't really tried to reproduce the issue (and compared the behavior with the modifications here) since this PR has already so many comments I hope someone did already :) |
That has been done, for both cases. I am still not fully convinced of the solution. I kept the order of stack iteration as the original, but have no proper explanation why one needs to start at the bottom instead of the top (as defined by However, I infer from all the checks that occur before we start obtaining the stack trace and before we start the first iteration, that the first loop will always succeed with a file found, for the following reasons:
So the second loop only exists by virtue of defensive programming.
That is a valid suggestion. Test methods tend to get similar names, and one name could be the prefix of another. The parentheses avoid that. |
This version works for ant projects.
…itNodeOpener.java Co-authored-by: Matthias Bläsing <mblaesing@doppel-helix.eu>
can correctly hit. Suggested by matthiasblaesing.
homberghp
changed the title
This version works for ant projects. Update java/junit.ant.ui/src/org/netbeans/modules/junit/ant/ui/AntJUnitNodeOpener.java Co-authored-by: Matthias Bläsing <mblaesing@doppel-helix.eu>
@matthiasblaesing Tried to apply a trick mbien once told me, but that does not seem to work anymore, or I do not know how to apply that in this case: git pull --rebase --autostash git@github.com:apache/netbeans master
git push -f git@github.com:homberghp/netbeans issue8567:issue8567
|
if something isn't working as intended, please don't force push into the PR branch. Force push is destructive. If git log doesn't show what you want it to show -> don't push. e.g the log is a bit confusing now. It seems to contain a squashed commit, but also a merge commit on top of it which adds the individual commits again (sometimes a symptom for merging in the wrong direction). Removing the merge commit does fix rebase. But: I don't know what is the latest revision. gh seems to have problems generating patch files too, since the patch contains more changes than the review tab. is this the file version we want? if yes: Detailsmake sure git status is clean and log is at the right hash now remove the merge commit and rebase again: the file is now identical to https://raw.githubusercontent.com/homberghp/netbeans/bae442be88e7095daef5b6269c4dace38b2deca2/java/junit.ant.ui/src/org/netbeans/modules/junit/ant/ui/AntJUnitNodeOpener.java and the commit is on top of the master hash. Now you may force push like you did before. |
5 participants
This version works for ant projects. (Including netbeans projects).
This PR closes #8567
This implementation of finding file object+line number reverses the search by starting at the top of the stack and try to find the stack frame that is directly related to the test method.
It reverses the loop running from bottom to top into running from the top stack frame to the bottom one.
It effectively tries to find the first stack element that matches the testMethod involved instead of the first element that is NOT in some (testing) framework. For most tests this appears to be the logical choice, in particular in the non-optimal practice of having multiple assert or assert-like statements in one test method.
For NetBeans developers, in particular those working on refactoring related stuff and tests, it is more useful to point to the failing assert or verifyXXX call than in some method inside some method inside the test class or a superclass thereof that generalizes repetitive work in the tests.
In the original the goto source landed me in
netbeans/java/refactoring.java/test/unit/src/org/netbeans/modules/refactoring/java/test/RefactoringTestBase.java
Lines 405 to 418 in 68e420d
instead of say line 118 below, which is the more logical line to choose in most cases.
netbeans/java/refactoring.java/test/unit/src/org/netbeans/modules/refactoring/java/test/InnerToOutterTest.java
Lines 117 to 118 in 68e420d
This PR solves the problem for both maven and ant projects in a similar way.