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 some tests and fix some exceptions during the tests #914

Merged
merged 10 commits into from
Oct 26, 2016
Merged

Add some tests and fix some exceptions during the tests #914

merged 10 commits into from
Oct 26, 2016

Conversation

xconverge
Copy link
Member

Cant run them all on my system, want to see if travis can...

Yay! We love PRs! 🎊

Please include a description of your change and ensure:

  • Commit message has a short title & issue references
  • Each commit does a logical chunk of work.
  • It builds and tests pass (e.g gulp)

More info can be found on our contribution guide.

@xconverge xconverge changed the title Add some tests Add some tests and fix some exceptions during the tests Oct 13, 2016
@johnfn
Copy link
Member

johnfn commented Oct 13, 2016

I'm all for new tests!

@@ -43,7 +43,18 @@ suite("Mode Insert", () => {
assertEqual(TextEditor.getSelection().start.character, 4, "<Esc> moved cursor position.");
});

test("", async () => {
test("<C-c> can exit insert", async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Wait, don't we need some special config for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's useCtrlKeys

Copy link
Member Author

@xconverge xconverge Oct 26, 2016

Choose a reason for hiding this comment

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

We have C-xxxx keys all throughout our tests though

@@ -54,6 +65,14 @@ suite("Mode Insert", () => {
return assertEqualLines(["text", ""]);
});

test("Stay in insert when entering characters", async () => {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this will ever catch the elusive bug unfortunately. It's triggered when there are multiple vimStates for the same file. That's almost impossible to cause in a test.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm.. A better test might be to switch the language to HTML or something and then ensure that there was only ever one mode handler created.

@@ -933,6 +933,20 @@ suite("Mode Normal", () => {
});

newTest({
title: "Can handle 'J' followed by x",
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

@rebornix this one is duplicated now right?

Copy link
Member

Choose a reason for hiding this comment

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

sorry it is :)


public async execActionForOperator(position: Position, vimState: VimState): Promise<Position | IMovement> {
const result = await this.execAction(position, vimState);
if (isIMovement(result)) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the duplication here. Ideally we don't even check for this case, it's just handled upstream in ModeHandler.

Copy link
Member Author

@xconverge xconverge Oct 25, 2016

Choose a reason for hiding this comment

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

by the way, these tests that I was trying to fix with these lines dont throw exceptions on macOS but they do on windows...

@johnfn
Copy link
Member

johnfn commented Oct 14, 2016

Oops, I wrote some comments a few days ago but they only just got added.

@rebornix rebornix merged commit 09af878 into VSCodeVim:master Oct 26, 2016
This pull request was closed.
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.

3 participants