Add just the test infrastructure bits from #4354 #4382

zadjii-msft · 2020-01-28T15:34:04Z

Summary of the Pull Request

#4354 is a pretty complicated PR. It's got a bunch of conpty changes, but what it also has was some critical improvements to the roundtrip test suite. I'm working on some other bugfixes in the same area currently, and need these tests enhancements in those branches now. The rest of #4354 is complex enough that I don't trust it will get merged soon (if ever). However, these fixes should be in regardless.

PR Checklist

Taken directly from When the Terminal is resized, don't remove lines from scrollback #4354
I work here
Tests added/passed
[n/a] Requires documentation to be updated

Detailed Description of the Pull Request / Additional comments

This is four main changes:

Enable conpty to be fully enabled in unittests. Just setting up a VT renderer isn't enough to trick the host into being in conpty mode - it also needs to have some other flags set.
Some minor changes to CommonState to better configure the common test state for conpty
Move some of the verify helpers from ConptyRoundtripTests into their own helper class, to be shared in multiple tests
Add a TerminalBufferTests class, for testing the Terminal buffer directly (without conpty).

This change is really easier than

would suggest, I promise.

zadjii-msft · 2020-01-28T17:09:59Z

/azp run

I really don't think I affected the CanMoveByWord_NonEmptyBuffer tests, so I'm just re-running. Curious that the failure here is different than the #4344 failure that's usually hitting.

azure-pipelines · 2020-01-28T17:10:13Z

Azure Pipelines successfully started running 1 pipeline(s).

miniksa · 2020-01-28T23:15:13Z

/azp run

azure-pipelines · 2020-01-28T23:15:24Z

Azure Pipelines successfully started running 1 pipeline(s).

miniksa · 2020-01-28T23:25:02Z

If this succeeds, it might just be the solution to #4384

miniksa · 2020-01-28T23:55:46Z

OK it succeeded twice. I think this might solve the x64 unit test failures then. Reading it fully now. @DHowett-MSFT

miniksa

This does indeed look straightforward. I don't super like the "fools the console into being in pty mode" defines sprayed around, but I don't have a better idea without either changing the entire initialization pipeline or something of that ilk. (Going to guess that you had the same mental debate and settled on this.) So I approve. Especially because it looks like this fixes the x64 unit test hang.

DHowett-MSFT · 2020-01-29T01:12:41Z

src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp


-    VERIFY_ARE_EQUAL(first.length(), cch);
-    VERIFY_ARE_EQUAL(first, actualString);
+        Log::Comment(NoThrowString().Format(L"Expected =\t\"%hs\"", first.c_str()));


will this print raw escapes, or escape them?

should we replace 0x1b with 0x241b?

DHowett-MSFT · 2020-01-29T01:12:52Z

src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp


-    VERIFY_ARE_EQUAL(first.length(), cch);
-    VERIFY_ARE_EQUAL(first, actualString);
+        Log::Comment(NoThrowString().Format(L"Expected =\t\"%hs\"", first.c_str()));


should we replace 0x1b with 0x241b?

zadjii-msft · 2020-01-29T13:33:19Z

/azp run

azure-pipelines · 2020-01-29T13:33:46Z

Azure Pipelines successfully started running 1 pipeline(s).

…onpty-test-fixes

ghost · 2020-01-29T14:30:50Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

zadjii-msft · 2020-01-29T14:32:33Z

For the record, the manual run I did above was just to give us more data if this does in fact fix #4344. The tests did pass, so it seems to be looking good. I don't know what I did, but if this next run passes too, lets just close #4344 as well.

zadjii-msft · 2020-01-29T15:19:17Z

WELP

/azp run

EDIT: why can't I just slap that into a comment and have it work? I want to be able to say something and also trigger a build ;__:

zadjii-msft · 2020-01-29T15:20:41Z

/azp run

azure-pipelines · 2020-01-29T15:20:52Z

Azure Pipelines successfully started running 1 pipeline(s).

zadjii-msft · 2020-01-29T16:10:23Z

Well https://dev.azure.com/ms/Terminal/_build/results?buildId=60592&view=logs&j=e7365288-57d7-5349-8dfa-9c195127bd77 is curious.

The x86 test pass already has errors with UiaTextRangeTests::CanMoveByWord_NonEmptyBuffer and UiaTextRangeTests::CanMoveEndpointByUnitWord. But it kept on chugging along till it got to the roundtrip/output tests, where it got stuck again, in x86 this time though. x64 already passed.

zadjii-msft · 2020-01-29T16:10:36Z

/azp run

azure-pipelines · 2020-01-29T16:10:48Z

Azure Pipelines successfully started running 1 pipeline(s).

Let's pull all the test fixes into their own file

ce3138c

zadjii-msft added Product-Terminal The new Windows Terminal. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. labels Jan 28, 2020

zadjii-msft requested review from carlos-zamora, DHowett-MSFT, miniksa and leonMSFT January 28, 2020 15:34

zadjii-msft mentioned this pull request Jan 28, 2020

A pair of fixes related to cursor movement in conpty #4372

Merged

5 tasks

microsoft deleted a comment from azure-pipelines bot Jan 28, 2020

zadjii-msft added a commit that referenced this pull request Jan 28, 2020

This is polished for PR, ready to go in after #4382

0755fd7

miniksa approved these changes Jan 29, 2020

View reviewed changes

DHowett-MSFT reviewed Jan 29, 2020

View reviewed changes

DHowett-MSFT approved these changes Jan 29, 2020

View reviewed changes

zadjii-msft added 2 commits January 29, 2020 07:51

Merge remote-tracking branch 'origin/master' into dev/migrie/b/just-c…

2daec48

…onpty-test-fixes

Do a string replace to escape the escape characters

96e16fd

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 29, 2020

zadjii-msft mentioned this pull request Jan 29, 2020

Some of the unittests are flaky in CI #4344

Closed

ghost merged commit 685720a into master Jan 29, 2020

ghost deleted the dev/migrie/b/just-conpty-test-fixes branch January 29, 2020 16:33

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add just the test infrastructure bits from #4354 #4382

Add just the test infrastructure bits from #4354 #4382

zadjii-msft commented Jan 28, 2020

zadjii-msft commented Jan 28, 2020 •

edited

Loading

azure-pipelines bot commented Jan 28, 2020

miniksa commented Jan 28, 2020

azure-pipelines bot commented Jan 28, 2020

miniksa commented Jan 28, 2020

miniksa commented Jan 28, 2020

miniksa left a comment

DHowett-MSFT Jan 29, 2020

DHowett-MSFT Jan 29, 2020

DHowett-MSFT Jan 29, 2020

zadjii-msft commented Jan 29, 2020

azure-pipelines bot commented Jan 29, 2020

ghost commented Jan 29, 2020

zadjii-msft commented Jan 29, 2020

zadjii-msft commented Jan 29, 2020 •

edited

Loading

zadjii-msft commented Jan 29, 2020

azure-pipelines bot commented Jan 29, 2020

zadjii-msft commented Jan 29, 2020

zadjii-msft commented Jan 29, 2020

azure-pipelines bot commented Jan 29, 2020

Add just the test infrastructure bits from #4354 #4382

Add just the test infrastructure bits from #4354 #4382

Conversation

zadjii-msft commented Jan 28, 2020

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

zadjii-msft commented Jan 28, 2020 • edited Loading

azure-pipelines bot commented Jan 28, 2020

miniksa commented Jan 28, 2020

azure-pipelines bot commented Jan 28, 2020

miniksa commented Jan 28, 2020

miniksa commented Jan 28, 2020

miniksa left a comment

Choose a reason for hiding this comment

DHowett-MSFT Jan 29, 2020

Choose a reason for hiding this comment

DHowett-MSFT Jan 29, 2020

Choose a reason for hiding this comment

DHowett-MSFT Jan 29, 2020

Choose a reason for hiding this comment

zadjii-msft commented Jan 29, 2020

azure-pipelines bot commented Jan 29, 2020

ghost commented Jan 29, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

zadjii-msft commented Jan 29, 2020

zadjii-msft commented Jan 29, 2020 • edited Loading

zadjii-msft commented Jan 29, 2020

azure-pipelines bot commented Jan 29, 2020

zadjii-msft commented Jan 29, 2020

zadjii-msft commented Jan 29, 2020

azure-pipelines bot commented Jan 29, 2020

zadjii-msft commented Jan 28, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

zadjii-msft commented Jan 29, 2020 •

edited

Loading