-
Notifications
You must be signed in to change notification settings - Fork 27
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
test: Added unit tests for LSP-based code folding and the code block provider (issue 673) #679
Conversation
…e class and updated the folding range tests to use them as well.
// if quick flag is set, we do nothing here | ||
if (quick) { | ||
// if quick flag is set and not testing, we do nothing here | ||
if (quick && !ApplicationManager.getApplication().isUnitTestMode()) { |
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.
Tests are run in dumb mode so if we want to test this, we need to allow that, at least for test execution.
@@ -219,6 +220,7 @@ protected boolean isRegionCollapsedByDefault(@NotNull ASTNode node) { | |||
|
|||
@Override | |||
public boolean isDumbAware() { | |||
return false; | |||
// Allow folding in dumb mode only during unit testing | |||
return ApplicationManager.getApplication().isUnitTestMode(); |
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.
Same thing here...allow dumb mode during test execution.
""", | ||
new int[][]{ | ||
// From "console" to the begin and end of the "demo()" function's braced block | ||
{beforeFirst(DEMO_TS_FILE_BODY, "console", 1), afterFirst(DEMO_TS_FILE_BODY, "{", 2), beforeLast(DEMO_TS_FILE_BODY, "}", 2)}, |
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.
Each of these test scenarios include the initial caret offset, the expected caret offset when move-to-block-start occurs, and the expected caret offset when move-to-block-end occurs. As you can see, I've added some utility methods to the common base test case class that allow us to derive those indices dynamically rather than hard-coding them, (hopefully) resulting in much more flexible tests if/when test file bodies change.
"""; | ||
|
||
// Demo class braced block exclusive of braces | ||
private static final TextRange DEMO_CLASS_BODY_TEXT_RANGE = TextRange.create(afterFirst(DEMO_TS_FILE_BODY, "{", 1), beforeLast(DEMO_TS_FILE_BODY, "}", 1)); |
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.
Using the same utility methods here to derive the expected text ranges dynamically.
@@ -18,15 +18,18 @@ | |||
*/ | |||
public class TypeScriptSelectionRangeTest extends LSPSelectionRangeFixtureTestCase { | |||
|
|||
private static final String DEMO_TS_FILE_NAME = "demo.ts"; |
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.
No new tests were added here, but I extracted common constants for repeated strings for consistency with what I did in the new tests added in this PR.
fail(e.getMessage()); | ||
} | ||
|
||
EditorTestUtil.buildInitialFoldingsInBackground(editor); |
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 is the important step required to trigger folding behavior in the file. You'll see it in both of the new base test case classes.
@@ -72,4 +73,85 @@ private void registerServer() { | |||
private void unregisterServer() { | |||
LanguageServersRegistry.getInstance().removeServerDefinition(myFixture.getProject(), serverDefinition); | |||
} | |||
|
|||
// Utility methods for getting the before/after index of the n'th occurrence of a substring of the test file body |
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.
Here are the utility methods referenced earlier.
fail(e.getMessage()); | ||
} | ||
|
||
EditorTestUtil.buildInitialFoldingsInBackground(editor); |
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.
Again, force population of the folding model in test mode.
"demo.ts", | ||
// language=typescript | ||
"console.log('message');", | ||
DEMO_TS_FILE_NAME, |
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 is just my opinion, but I find using constant is not easy to read, because you need to jump to the constant to know the value.
But if you prefer , keep like this.
] | ||
""", | ||
new int[][]{ | ||
// From "console" to the begin and end of the "demo()" function's braced block |
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.
Instead of expecting some offset, I think it will more readable to expect a string which have the same content of the editor but with some annotations like <fold1>
ends with </fold1>
It follows the same idea with the <caret>
that you can use in a test (see for instance completion test, where I use <caret]>
instead of using the offset number see, for instance
lsp4ij/src/test/java/com/redhat/devtools/lsp4ij/features/completion/TypeScriptCompletionTest.java
Line 55 in 254cf2e
assertApplyCompletionItem(0, "''.charAt<caret>"); |
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.
Here a sample with folding ranges:
public void testFoldingRanges() {
assertFoldingRanges(
"demo.ts",
"""
export class Demo {
demo() {
console.log('demo');
}
}
""",
// language=json
"""
[
{
"startLine": 0,
"endLine": 3
},
{
"startLine": 1,
"endLine": 2
}
]
""",
"""
<fold1>export class Demo {
demo() {
console.log('demo');
}
}</fold1>
"""
);
}
}
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've changed the tests to provide a balance between readability and maintainability. Tokens are now used, but directly in the test file source, and they're both extracted for offset information and stripped from the file body for the actual PsiFile
creation. That way you're only maintaining one version of the file body with tokens representing key locations.
Let me know if that doesn't address your concerns.
…per-invocation now.
Impressive! |
The title is pretty self-explanatory. The only thing worth pointing out explicitly is that I've added some utility methods to the common base test case class for deriving the before/after index of specific substrings in test file bodies to avoid having to hard-code them.