Skip to content

Increase timeout for TestPseudoConsolePowershell#1253

Closed
dcantah wants to merge 1 commit intomicrosoft:masterfrom
dcantah:fix-execconpty-test
Closed

Increase timeout for TestPseudoConsolePowershell#1253
dcantah wants to merge 1 commit intomicrosoft:masterfrom
dcantah:fix-execconpty-test

Conversation

@dcantah
Copy link
Copy Markdown
Contributor

@dcantah dcantah commented Dec 22, 2021

On the CI machines I'd seen an instance of this hitting the timeout. The
timeout in place may have been too optimistic so this change ups the
timeout to receive the output to 10 seconds from 5.

Signed-off-by: Daniel Canter dcanter@microsoft.com

On the CI machines I'd seen an instance of this hitting the timeout. The
timeout in place may have been too optimistic so this change ups the
timeout to receive the output to 10 seconds from 5.

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
@helsaawy
Copy link
Copy Markdown
Contributor

It takes ~10 seconds for the conpty to forward the output, or is part of the delay the powershell's startup?

@dcantah
Copy link
Copy Markdown
Contributor Author

dcantah commented Dec 22, 2021

@helsaawy powershell startup is what I'm thinking. Let me check on my machine at least when the first output is received on average

@@ -207,14 +207,14 @@ func TestPseudoConsolePowershell(t *testing.T) {
}()

cmd := "echo \"howdy from conpty\"\r\n"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small nit, can we make "howdy from conpty" a const and use that on line 201 and format it into this string?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure!

@helsaawy
Copy link
Copy Markdown
Contributor

If its startup and we really care about timing, we may want to add a wait (with a timeout) earlier to check for proper init first, then use a smaller timeout for the forwarding.
But, regardless of that and my small nit, this lgtm

@dcantah
Copy link
Copy Markdown
Contributor Author

dcantah commented Dec 22, 2021

Oh actually, the reason for the failure might be due to a read containing only part of the string we're looking for. Gonna close this and come up with a better route

@dcantah dcantah closed this Dec 22, 2021
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