-
Notifications
You must be signed in to change notification settings - Fork 93
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
NPE on hover on malformed document #985
Conversation
Fixes eclipse-lemminx#984 Signed-off-by: azerr <[email protected]>
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 think this is the correct solution, but the test doesn't actually reproduce the issue from what I can tell.
@@ -304,6 +304,11 @@ public void hoverTextWithUnion3() throws BadLocationException, MalformedURIExcep | |||
"Source: [dressSize.xsd](" + schemaURI + ")", | |||
r(2, 52, 3, 9)); | |||
} | |||
|
|||
@Test | |||
public void hoverText() throws BadLocationException { |
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.
This test passes even if the null check isn't present.
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.
It's because exception is cached, so we don't see the problem. But this PR clean the usecase and doesn't throw the NPE exception.
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.
Okay. I don't see the point of including this test case if it doesn't reproduce the issue.
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 would like to keep it because without this fix we see a error stack trace when test is executed.
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.
Okay. I can see why this would be helpful, but its very easy to miss this exception in the output. I think it would be better to write a unit test on ContentModelHoverParticipant.onText()
so that the build will fail if this behaviour is changed.
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 think a test like this would be more helpful:
@Test
public void hoverWithNullParentNode() throws Exception {
ContentModelHoverParticipant hoverParticipant = new ContentModelHoverParticipant();
IHoverRequest hoverRequest = new IHoverRequest() {
// default auto-suggested implementation for other methods
@Override
public DOMNode getNode() {
return new DOMText(0, 0);
}
};
assertNull(hoverParticipant.onText(hoverRequest));
}
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.
See #1001
Replace with #1001 |
NPE on hover on malformed document
Fixes #984
Signed-off-by: azerr [email protected]