-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix #2148: vertical split command #2158
Conversation
Fix the vertical split commands, which were broken when the logic for the showOpenDialog command was added by PR VSCodeVim#2067.
Add unit tests for both :vs and :vsp commands. These tests are heavily based on the writequit.test.
test/cmd_line/vsplit.test.ts
Outdated
|
||
let pollCount = 0; | ||
// TODO: the visibleTextEditors variable doesn't seem to be | ||
// up to date after a onDidChangeActiveTextEditor event, not |
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 you want the onDidChangeVisibleTextEditors
event here
test/cmd_line/vsplit.test.ts
Outdated
import * as assert from 'assert'; | ||
import { getAndUpdateModeHandler } from '../../extension'; | ||
|
||
async function WaitForEditorOpen(): Promise<void> { |
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.
Consider making this function a bit more generic. Maybe accept an "expectedEditorOpen" param.
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.
Thanks @westim. Left a couple of comments.
The vertical split tests were formerly polling the window, waiting for the number of text editors to change. This is now triggered based on the vscode.window.onDidChangeVisibleTextEditors event. Also, a setup/teardown bug was fixed so the setup and teardown is performed for each test, not once for the suite.
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.
One further comment, but LGTM otherwise.
test/cmd_line/vsplit.test.ts
Outdated
* | ||
* @param numEditors Expected number of editors in the window | ||
*/ | ||
async function WaitForEditors(numEditors: number): Promise<void> { |
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.
Can you move this to testUtils
? There's probably value in sharing it with other test classes.
Thanks @westim |
This PR fixes the vertical split command, which was a regression caused by PR #2067.
I have added unit tests for the
:vs
and:vsp
commands.