-
Notifications
You must be signed in to change notification settings - Fork 49
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
Filtering unqualified Stacktraces #115 #547
Filtering unqualified Stacktraces #115 #547
Conversation
Hi @iloveeclipse & @jukzi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try to write code that "fits on the screen".
Methods that have nesting level like here and if blocks that need to be scrolled down to see where it ends are "no go". The code is impossible to review / maintain.
See my proposals in code how the code should be improved.
...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/console/JavaStackTraceHyperlink.java
Outdated
Show resolved
Hide resolved
...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/console/JavaStackTraceHyperlink.java
Outdated
Show resolved
Hide resolved
...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/console/JavaStackTraceHyperlink.java
Outdated
Show resolved
Hide resolved
...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/console/JavaStackTraceHyperlink.java
Outdated
Show resolved
Hide resolved
...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/console/JavaStackTraceHyperlink.java
Outdated
Show resolved
Hide resolved
...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/console/JavaStackTraceHyperlink.java
Outdated
Show resolved
Hide resolved
...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/console/JavaStackTraceHyperlink.java
Show resolved
Hide resolved
Thanks for the proposals @iloveeclipse, now I have decoupled it into many like you suggested and it is more readable than previous. |
I'm away from PC till next week, code looks much better now, thanks. |
Thank you @iloveeclipse |
I have tested this pr with a hundret random stacktraces and did not run into any error. @iloveeclipse please finish review. |
8f84fea
to
8e6f576
Compare
Hi @iloveeclipse, could u pls check this again ? |
|
I will. Please squash two commits into one & force push this PR. |
Thanks @iloveeclipse, will do it now |
8e6f576
to
60d19e6
Compare
Hi @iloveeclipse , I have squashed the commits to one now. |
60d19e6
to
3a15766
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please check the coverage of the new code, it suggests that the code either not needed or there are no tests added:
- For this line
at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:109)
I get an error in the log and a selection dialog for two types, one of them isbin.org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher
:
Java Model Exception: Error in Java Model (code 969): bin.org.eclipse.core.runtime.internal.adaptor [in /data/git/equinox/bundles/org.eclipse.osgi/bin] does not exist
at org.eclipse.jdt.internal.core.JavaElement.newJavaModelException(JavaElement.java:556)
at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:234)
at org.eclipse.jdt.internal.core.Openable.openAncestors(Openable.java:508)
at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:228)
at org.eclipse.jdt.internal.core.JavaElement.openWhenClosed(JavaElement.java:569)
at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:292)
at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:278)
at org.eclipse.jdt.internal.core.BinaryType.getClassFileInfo(BinaryType.java:232)
at org.eclipse.jdt.internal.core.BinaryType.getChildren(BinaryType.java:195)
at org.eclipse.jdt.internal.core.JavaElement.getChildrenOfType(JavaElement.java:247)
at org.eclipse.jdt.internal.core.BinaryType.getMethods(BinaryType.java:494)
at org.eclipse.jdt.internal.debug.ui.console.JavaStackTraceHyperlink.extractFromResults(JavaStackTraceHyperlink.java:316)
at org.eclipse.jdt.internal.debug.ui.console.JavaStackTraceHyperlink.filterClasses(JavaStackTraceHyperlink.java:245)
at org.eclipse.jdt.internal.debug.ui.console.JavaStackTraceHyperlink$3.runInUIThread(JavaStackTraceHyperlink.java:208)
at org.eclipse.ui.progress.UIJob.lambda$0(UIJob.java:148)
On master, I don't get an error (but still see unexpected bin.org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher
type proposed in a list with the "right" one.
...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/console/JavaStackTraceHyperlink.java
Outdated
Show resolved
Hide resolved
...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/console/JavaStackTraceHyperlink.java
Outdated
Show resolved
Hide resolved
...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/console/JavaStackTraceHyperlink.java
Outdated
Show resolved
Hide resolved
...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/console/JavaStackTraceHyperlink.java
Outdated
Show resolved
Hide resolved
...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/console/JavaStackTraceHyperlink.java
Outdated
Show resolved
Hide resolved
...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/console/JavaStackTraceHyperlink.java
Outdated
Show resolved
Hide resolved
|
3a15766
to
f595ab8
Compare
b8fc106
to
0f2ebc7
Compare
Hi @iloveeclipse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no time left today for the review due other duties, and will be back at the keyboard next Monday again, sorry.
I saw the coverage is increased, this is good. However, I also see the hack for handling broken "bin" types and that should be removed / fixed properly in a different PR.
Please note, you have again two commits on the branch - it should be one.
...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/console/JavaStackTraceHyperlink.java
Outdated
Show resolved
Hide resolved
No problem.. will clean up everything 👍 |
be2ed34
to
e1efb4f
Compare
I have done the changes and squashed the commits to one now. |
e1efb4f
to
7e367e3
Compare
Hi @iloveeclipse, for the additional bin file pickup in results I wasn't able to reproduce the same in my local, I'm always getting results without bin. Can you check that again ? |
7e367e3
to
139e5d6
Compare
This pull request changes some projects for the first time in this development cycle.
An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patch
Further information are available in Common Build Issues - Missing version increments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest patch has 78% test coverage and all but one new methods seem to be tested, this is good. Also I've tested few stack traces without errors - for the first time :-) Also good.
So if @jukzi is OK with this patch, let's merge.
since there have been some changes since my last test, i would like to test this tomorrow once more. |
fd75f82
to
600dc85
Compare
got 81% coverage now |
Build repeatedly fails with
|
It's infrastructure issue - I've opened https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/5387 for another case but feel free to open a new one or comment to this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested some stacktraces. Most work. But it fails on some (i used the stacktraces that failed the build of this PR :-) ):
00:01:35.805 at jdk.internal.reflect.DirectMethodHandleAccessor.invoke (DirectMethodHandleAccessor.java:103)
00:01:35.805 at java.lang.reflect.Method.invoke (Method.java:580)
00:01:35.805 at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced (Launcher.java:255)
00:01:35.805 at org.codehaus.plexus.classworlds.launcher.Launcher.launch (Launcher.java:201)
00:01:35.805 at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode (Launcher.java:361)
00:01:35.805 at org.codehaus.plexus.classworlds.launcher.Launcher.main (Launcher.java:314)
!ENTRY org.eclipse.ui.workbench 4 0 2024-12-04 09:09:04.873
!MESSAGE An internal error has occurred.
!STACK 0
java.lang.StringIndexOutOfBoundsException: Range [0, -1) out of bounds for length 9
at java.base/jdk.internal.util.Preconditions$1.apply(Preconditions.java:55)
at java.base/jdk.internal.util.Preconditions$1.apply(Preconditions.java:52)
at java.base/jdk.internal.util.Preconditions$4.apply(Preconditions.java:213)
at java.base/jdk.internal.util.Preconditions$4.apply(Preconditions.java:210)
at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:98)
at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckFromToIndex(Preconditions.java:112)
at java.base/jdk.internal.util.Preconditions.checkFromToIndex(Preconditions.java:349)
at java.base/java.lang.String.checkBoundsBeginEnd(String.java:4865)
at java.base/java.lang.String.substring(String.java:2834)
at org.eclipse.jdt.internal.debug.ui.console.JavaStackTraceHyperlink$3.runInUIThread(JavaStackTraceHyperlink.java:213)
instead of using low level methods like substring, indexof you would be better of using pattern matching as in org.eclipse.jdt.internal.debug.ui.actions.OpenFromClipboardAction
You may also want to improve error handling to add the text to the exception that could not be parsed.
will check this 👍 |
4a53e8b
to
17c4525
Compare
Hi @jukzi , I have replaced all the indexof & substring in critical sections and handled new stack traces (those new traces wont give proper generatedLink)
for this I should make changes in platform ui ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested the change again. I could not find any error anymore. potential multithreading issues have to be fixed, then its good to go from my side.
...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/console/JavaStackTraceHyperlink.java
Outdated
Show resolved
Hide resolved
...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/console/JavaStackTraceHyperlink.java
Show resolved
Hide resolved
...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/console/JavaStackTraceHyperlink.java
Outdated
Show resolved
Hide resolved
ecf4c77
to
f84f53f
Compare
...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/console/JavaStackTraceHyperlink.java
Outdated
Show resolved
Hide resolved
...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/console/JavaStackTraceHyperlink.java
Outdated
Show resolved
Hide resolved
f84f53f
to
a4afc35
Compare
Using method name to filter unqualified stacktraces generated in java stacktrace console to provide more accurate search results. Fixes eclipse-jdt#115
a4afc35
to
249de72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, lets wait for the build
Build completed without issues. |
Thanks for the reminder and contributions. |
Thank you @iloveeclipse & @jukzi for the detailed review. |
using method name to filter unqualified stacktraces generated in java stacktrace console.
Fixes #115
Includes handling for #544
What it does
How to test
Author checklist