Fix many vt tests #500

Tyriar · 2017-01-21T08:11:51Z

This PR fixes many of the tests in escape-sequence-tests.js, see the commits for details on the individual changes. Previously 10 tests were passing, now 30+ are.

The major fixes:

CAN has been implemented which cancels any escape sequence, the corresponding test does not work still due to complex ignore logic in xterm that we don't have parity with
Index, LF/VT/FF, CUB, CUD, VPR behavior was change to stay at the end of the column if it was otherwise going to go to wraparound
When wraparound mode is disabled output will be cut off the end, no longer will the last character always be displayed in the last column
There was an error which cause causing our tests to compare only the first page of the buffer, not the viewport.
Expected output was trimmed as we don't care, at least not currently (we trim all output from xterm.js anyway).

There is a failing test:

1) xterm.js unicode - surrogates 2 characters per cell over line end without autowrap:

      AssertionError: expected 'a' to deeply equal '𐀀'
      + expected - actual

      -a
      +𐀀

This is likely because it relied on the previous wraparound disabled logic which I believe was wrong. @jerch thoughts since you added it?

This fixes 2 tests

Don't wrap cursor to column 0 after an index, fixes 2 tests

This helps partially pass t0014-CAN.in, it won't work 100% however due to the very complex parsing logic of vanilla xterm that gnome-terminal doesn't even seem to match. xterm's source seems to have multiple ignore cases for a set of characters such that an escape sequence like \e!!!!!! prints nothing as the ! continually gets ignored. Our current logic is we ignore the escape sequence immediately in this case and print the !.

Don't wrap cursor to column 0 after a CUB, fixes 3 tests

Don't wrap cursor to column 0 after a CUD, fixes 1 test

Don't wrap cursor to column 0 after a VPR, fixes 1 test

parisk

Just some documentation requests and after fixing the test we are 👍 .

parisk · 2017-01-21T08:46:27Z

src/InputHandler.ts

@@ -124,6 +123,9 @@ export class InputHandler implements IInputHandler {
      this._terminal.y--;
      this._terminal.scroll();
    }
+    if (this._terminal.x >= this._terminal.cols) {


What is the role of these lines here?

Can you add a line comment please?

Seems like you are shifting things up when you are inserting a line feed, when you are in the bottom line?

parisk · 2017-01-21T08:49:36Z

src/InputHandler.ts

@@ -218,6 +220,9 @@ export class InputHandler implements IInputHandler {
    if (this._terminal.y >= this._terminal.rows) {
      this._terminal.y = this._terminal.rows - 1;
    }
+    if (this._terminal.x >= this._terminal.cols) {


Can you add a line comment of what this if clause does please? Seems identical to https://github.com/sourcelair/xterm.js/pull/500/files#r97194377.

parisk · 2017-01-21T08:49:43Z

src/InputHandler.ts

@@ -244,6 +249,9 @@ export class InputHandler implements IInputHandler {
    if (param < 1) {
      param = 1;
    }
+    if (this._terminal.x >= this._terminal.cols) {


Can you add a line comment of what this if clause does please? Seems identical to https://github.com/sourcelair/xterm.js/pull/500/files#r97194377.

parisk · 2017-01-21T08:49:46Z

src/InputHandler.ts

@@ -693,6 +701,9 @@ export class InputHandler implements IInputHandler {
    if (this._terminal.y >= this._terminal.rows) {
      this._terminal.y = this._terminal.rows - 1;
    }
+    if (this._terminal.x >= this._terminal.cols) {


Can you add a line comment of what this if clause does please? Seems identical to https://github.com/sourcelair/xterm.js/pull/500/files#r97194377.

parisk · 2017-01-21T08:49:52Z

src/xterm.js

@@ -2303,6 +2303,9 @@ Terminal.prototype.index = function() {
    this.y--;
    this.scroll();
  }
+  if (this.x >= this.cols) {


Can you add a line comment of what this if clause does please? Seems identical to https://github.com/sourcelair/xterm.js/pull/500/files#r97194377.

parisk · 2017-01-21T08:58:45Z

This is the test case failing: https://github.com/sourcelair/xterm.js/blob/4d5046b5e7047bfcf1f1dba088acb343df1969bb/src/test/test.js#L598

Not sure which of these three expects fail. I think that you can just uncomment all your changes in inputhandler, parser and xterm and then by putting one piece at a time back in you will be able to find what broke this test.

parisk · 2017-01-21T08:58:55Z

Any way I can help?

Tyriar · 2017-01-21T16:23:05Z

It's the first expect and I think it's to do with Tyriar@43c3349

I believe it's verifying the wrong behavior.

jerch · 2017-01-21T18:37:18Z

@Tyriar Yeah the test checks that with wraparoundMode = false surrogate chars correctly end up the last cell. Since you changed that behavior to truncating the line, the test is not applicable anymore.
(Btw truncating the line is not conform to the old VTs, they were not able to move the cursor beyond the display, thats why they simply inserted the char at the last cell)

Tyriar · 2017-01-21T18:41:06Z

@jerch thanks for the clarification, I'll pull the test out 😄

Tyriar · 2017-01-25T18:35:04Z

@parisk good to look at again

parisk

LGTM 🚀

Tyriar added 11 commits January 20, 2017 21:20

Prevent wraparound mode from overriding last character

43c3349

Enable other tests and fix viewport check

d5f0fff

Right trim expected output, we don't care for now

adaf604

This fixes 2 tests

Fix index behavior

7206332

Don't wrap cursor to column 0 after an index, fixes 2 tests

Fix LF/VT/FF behavior

99fbcd3

Don't wrap cursor to column 0 after an index, fixes 2 tests

Fix CUB behavior

d30fb89

Don't wrap cursor to column 0 after a CUB, fixes 3 tests

Fix CUD behavior

74c3a4a

Don't wrap cursor to column 0 after a CUD, fixes 1 test

Fix VPR behavior

2aa7608

Don't wrap cursor to column 0 after a VPR, fixes 1 test

Mock viewport.syncScrollArea

da16dd7

Skip broken tests

4f7ee7d

Tyriar self-assigned this Jan 21, 2017

Tyriar requested a review from parisk January 21, 2017 08:11

parisk suggested changes Jan 21, 2017

View reviewed changes

Tyriar added 2 commits January 25, 2017 10:32

Fix failing test given new auto wraparound behavior

b789a7e

Add comments explaining actions preventing x from wrapping

5850cbc

parisk approved these changes Jan 26, 2017

View reviewed changes

Tyriar merged commit d1ad8a7 into xtermjs:master Jan 26, 2017

Tyriar deleted the fix_vt_tests branch January 26, 2017 16:55

Tyriar modified the milestone: 2.3.0 Feb 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix many vt tests #500

Fix many vt tests #500

Tyriar commented Jan 21, 2017 •

edited

parisk left a comment

parisk Jan 21, 2017

parisk Jan 21, 2017

parisk Jan 21, 2017

parisk Jan 21, 2017

parisk Jan 21, 2017

parisk Jan 21, 2017

parisk Jan 21, 2017

parisk commented Jan 21, 2017

parisk commented Jan 21, 2017

Tyriar commented Jan 21, 2017

jerch commented Jan 21, 2017

Tyriar commented Jan 21, 2017

Tyriar commented Jan 25, 2017

parisk left a comment

Fix many vt tests #500

Fix many vt tests #500

Conversation

Tyriar commented Jan 21, 2017 • edited

parisk left a comment

Choose a reason for hiding this comment

parisk Jan 21, 2017

Choose a reason for hiding this comment

parisk Jan 21, 2017

Choose a reason for hiding this comment

parisk Jan 21, 2017

Choose a reason for hiding this comment

parisk Jan 21, 2017

Choose a reason for hiding this comment

parisk Jan 21, 2017

Choose a reason for hiding this comment

parisk Jan 21, 2017

Choose a reason for hiding this comment

parisk Jan 21, 2017

Choose a reason for hiding this comment

parisk commented Jan 21, 2017

parisk commented Jan 21, 2017

Tyriar commented Jan 21, 2017

jerch commented Jan 21, 2017

Tyriar commented Jan 21, 2017

Tyriar commented Jan 25, 2017

parisk left a comment

Choose a reason for hiding this comment

Tyriar commented Jan 21, 2017 •

edited