Skip to content
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

Add test for TreeLineTracker.getLineInformation #1228

Merged
merged 2 commits into from
Jun 21, 2022

Conversation

lacinoire
Copy link
Contributor

@lacinoire lacinoire commented Jun 15, 2022

Hey 😊
I want to contribute the following test:

Test that a org.eclipse.lemminx.commons.BadLocationException is thrown when the parameter line of Position.<init> is set to -1.
This tests the method TreeLineTracker.getLineInformation.
This test is based on the test testEmptyDocumentInc.

Curious to hear what you think!

(I wrote this test as part of a research study at TU Delft. Find out more)

Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing. Hopefully the ECA process wasn't too much of a pain.

Quick question, did you create the change in Eclipse, VS Code, or neither ? I ask because when I format in Eclipse (and probably VS Code with Java support) it will re-adjust it so () -> gets placed right after BadLocationException.class, . The lambda function body is then indented only one level past the Asserts.assertThrows....

I'm not a committer here so @datho7561 can have a look. If you're fine with the change after the suggested update (inline), then let's merge.

document.setIncremental(true);
Position position = document.positionAt(0);
position = new Position(-1, 0);
int offset = document.offsetAt(position);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would avoid assigning the return statement to offset mainly because we're not using it. Just calling the method on the RHS as you're doing is enough to trigger the behaviour we want to test. That's the only (minor) improvement. I like the usage of assertThrows. Looks much cleaner.

() -> {
TextDocument document = new TextDocument("", "");
document.setIncremental(true);
Position position = document.positionAt(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also just hit me that positionAt also throws BadLocationException, and if that did happen, this would just pass. I would just remove this line and have Position position = new Position(-1, 0);

@datho7561 datho7561 self-requested a review June 15, 2022 18:40
Copy link
Contributor

@datho7561 datho7561 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! Once you've addressed Roland's comments, I think it should be good to merge.

@lacinoire
Copy link
Contributor Author

Thank you for the detailed feedback! 😄
I've incorporated all three changes as suggested.

For the formatting @rgrunber :
I'm using IntelliJ. Though the () -> { was already put on a new line during the test creation (by the pretty-printer of Spoon afaik), so I assume that confused IntelliJ into the large indent.
Changed it manually to your suggestions & fits better with the rest of the class now 🙂

@@ -155,6 +156,16 @@ public void testEmptyDocumentInc() throws BadLocationException {

}

@Test
public void testGetLineInformation() throws BadLocationException {
Copy link
Contributor

@angelozerr angelozerr Jun 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rename this test with testPositionAsBadLocation

@angelozerr angelozerr merged commit 22d0cff into eclipse-lemminx:master Jun 21, 2022
@angelozerr
Copy link
Contributor

Thanks so much @lacinoire for your contribution!

@angelozerr angelozerr added this to the 0.21.0 milestone Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants