Create tests that roundtrip output through a conpty to a Terminal by zadjii-msft · Pull Request #4213 · microsoft/terminal

zadjii-msft · 2020-01-14T16:57:01Z

Summary of the Pull Request

This PR adds two tests:

First, I started by writing a test where I could write output to the console host and inspect what output came out of conpty. This is the ConptyOutputTests in the host unit tests.
Then I got crazy and thought "what if I could take that output and dump it straight into the Terminal"? Hence, the ConptyRoundtripTests were born, into the TerminalCore unit tests.

References

Done in pursuit of #4200, but I felt this warranted it's own atomic PR

PR Checklist

Doesn't close anything on it's own.
I work here
you better believe this adds tests
[n/a] Requires documentation to be updated

Detailed Description of the Pull Request / Additional comments

From the comment in ConptyRoundtripTests:

This test class creates an in-proc conpty host as well as a Terminal, to
validate that strings written to the conpty create the same resopnse on the
terminal end. Tests can be written that validate both the contents of the
host buffer as well as the terminal buffer. Everytime that
renderer.PaintFrame() is called, the tests will validate the expected
output, and then flush the output of the VtEngine straight to th

Also, some other bits had to be updated:

The renderer needed to be able to survive without a thread, so I hadded a simple check that it actually had a thread before calling pThread->NotifyPaint
Bits in CommonState used NTSTATUS_FROM_HRESULT which did not work outside the host project. Since the NTSTATUS didn't seem that important, I replaced that with a HRESULT
CommonState likes to initialize the console to some weird defaults. I added an optional param to let us just use the defaults.

…he terminal

carlos-zamora

I'm guessing there's no way to reuse the same tests without duplicate code?

src/host/ut_host/ConptyOutputTests.cpp

src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp

…hich are blocked on #4213

zadjii-msft · 2020-01-14T22:53:26Z

I'm guessing there's no way to reuse the same tests without duplicate code?

At this point maybe but I kinda expect these tests to drift more over time, where re-using the code would make letting the tests drift apart and specialize harder.

I guess I could just not have the ConptyOutputTests at all, and just use the roundtrip ones, though I think I wanted to really focus on the raw output in the ConptyOutputTests, and focus on the results in ConptyRoundtripTests

Co-Authored-By: Carlos Zamora <[email protected]>

miniksa

Awesome. Glad to have these coming online. Just a few minor comments.

src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp

src/host/ut_host/ConptyOutputTests.cpp

…-these-tests

miniksa

Excellent, thanks. I'll try to fix the build issue given it's probably merge related and a bit late for you today.

ghost · 2020-01-17T00:22:22Z

Hello @miniksa!

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.

DHowett-MSFT · 2020-01-17T00:23:44Z

src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp

+        // STEP 2: Set up the Conpty
+
+        // Set up some sane defaults
+        auto& g = ServiceLocator::LocateGlobals();


Oh i am very deeply concerned that the TerminalCore tests have a dependency on CONSOLE_INFORMATION 😦

I mean if it's going to straddle looking at conpty and terminal... it sort of has to. The alternative is breaking it into YET ANOTHER library.

Yea it's kinda the whole point of this particular test. Yes I could move this to it's own project, but is that worth it?

miniksa · 2020-01-17T00:40:18Z

Ugh it's still toast somehow. Unmarking automerge. Now I have to go for the day so @zadjii-msft will probably see this in the morning before me.

…-these-tests

zadjii-msft · 2020-01-17T15:45:14Z

tests got stuck 😢 /azp run

zadjii-msft · 2020-01-17T16:17:01Z

/azp run

azure-pipelines · 2020-01-17T16:17:13Z

Azure Pipelines successfully started running 1 pipeline(s).

…inal (microsoft#4213)" This reverts commit 62765f1.

## Summary of the Pull Request In #4213 I added a dependency to the `UnitTests_TerminalCore` project on basically all of conhost. This _worked on my machine_, but it's consistently not working on other machines. This should fix those issues. ## References ## PR Checklist * [x] Closes #4285 * [x] I work here * [n/a] Tests added/passed * [n/a] Requires documentation to be updated ## Validation Steps Performed Made a fresh clone and built it.

zadjii-msft added 2 commits January 14, 2020 10:32

Create some tests that roundtrip content through conpt to verify in t…

302e04a

…he terminal

polish for review

bf4f610

zadjii-msft requested review from DHowett-MSFT, carlos-zamora, leonMSFT and miniksa January 14, 2020 16:57

zadjii-msft added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Product-Conpty For console issues specifically related to conpty Product-Terminal The new Windows Terminal. labels Jan 14, 2020

carlos-zamora reviewed Jan 14, 2020

View reviewed changes

src/host/ut_host/ConptyOutputTests.cpp Outdated Show resolved Hide resolved

src/host/ut_host/ConptyOutputTests.cpp Outdated Show resolved Hide resolved

src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp Outdated Show resolved Hide resolved

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

This is some cleanup, almost ready for PR but I need to write tests w…

416be46

…hich are blocked on #4213

Apply suggestions from code review

eb6992e

Co-Authored-By: Carlos Zamora <[email protected]>

miniksa reviewed Jan 16, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into dev/migrie/f/i-love…

a8fd596

…-these-tests

zadjii-msft mentioned this pull request Jan 16, 2020

Investigate include order issue with InputBuffer, readData #4262

Open

These were some comments from niksa

77fbfc6

miniksa approved these changes Jan 17, 2020

View reviewed changes

Merge branch 'master' into dev/migrie/f/i-love-these-tests

1b767b4

miniksa added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 17, 2020

DHowett-MSFT approved these changes Jan 17, 2020

View reviewed changes

miniksa removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 17, 2020

zadjii-msft added 2 commits January 17, 2020 09:05

Merge remote-tracking branch 'origin/master' into dev/migrie/f/i-love…

e2a918e

…-these-tests

fix @miniksa breaking my build

7c8a7d1

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

ghost merged commit 62765f1 into master Jan 17, 2020

ghost deleted the dev/migrie/f/i-love-these-tests branch January 17, 2020 16:40

j4james mentioned this pull request Jan 17, 2020

Build fails after commit 62765f1 #4285

Closed

mkitzan added a commit to mkitzan/terminal that referenced this pull request Jan 18, 2020

Revert "Create tests that roundtrip output through a conpty to a Term…

3b6e5c5

…inal (microsoft#4213)" This reverts commit 62765f1.

zadjii-msft mentioned this pull request Jan 21, 2020

Fix the UnitTests_TerminalCore's dependency on RendererGdi #4319

Merged

2 tasks

This pull request was closed.

Conversation

zadjii-msft commented Jan 14, 2020

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zadjii-msft commented Jan 14, 2020

Uh oh!

miniksa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miniksa left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jan 17, 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.

Uh oh!

DHowett-MSFT Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

miniksa Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

miniksa commented Jan 17, 2020

Uh oh!

zadjii-msft commented Jan 17, 2020

Uh oh!

zadjii-msft commented Jan 17, 2020

Uh oh!

azure-pipelines bot commented Jan 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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.