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 option for timeout and custom procdump #2976

Closed
wants to merge 7 commits into from

Conversation

nohwnd
Copy link
Member

@nohwnd nohwnd commented Jul 9, 2021

DO NOT MERGE, this is for experiments only

Description

VSTEST_TESTHOST_TIMEOUT to set how many ms we will wait till killing testhost
VSTEST_DUMP_PROCDUMPARGS to provide custom procdump parameters

Related issue

Kindly link any related issues. E.g. Fixes #xyz.

@@ -163,6 +163,10 @@ This test may, or may not be the source of the crash.</value>
<data name="Seconds" xml:space="preserve">
<value>seconds</value>
</data>
<data name="Terminatin" xml:space="preserve">
Copy link
Member Author

Choose a reason for hiding this comment

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

Terminating

Copy link
Member

Choose a reason for hiding this comment

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

Seems unused so far. Also, the resource key is using ing but the description is using ed we may want to synchronize that.

Copy link
Contributor

@MarcoRossignoli MarcoRossignoli left a comment

Choose a reason for hiding this comment

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

LGTM thx

@@ -320,8 +322,10 @@ public virtual void Close()
finally
{
this.initialized = false;

EqtTrace.Warning("ProxyOperationManager: Timed out waiting for test host to exit. Will terminate process.");
if (testHostExitedWithinTimeout == false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (testHostExitedWithinTimeout == false)
if (!testHostExitedWithinTimeout)

but I like it if it's done on purpose so feel free to keep as-is

Copy link
Member

Choose a reason for hiding this comment

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

The variable is nullable so you cannot simply use not.

this.RequestSender.EndSession();

// We want to give test host a chance to safely close.
// The upper bound for wait should be 100ms.
var timeout = 100;
var timeout = int.TryParse(Environment.GetEnvironmentVariable("VSTEST_TESTHOST_TIMEOUT"), out var t) ? t : 100;
Copy link
Member

Choose a reason for hiding this comment

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

Variable was already created before but could we rename it into shutdownTimeoutInMs?

@@ -320,8 +322,10 @@ public virtual void Close()
finally
{
this.initialized = false;

EqtTrace.Warning("ProxyOperationManager: Timed out waiting for test host to exit. Will terminate process.");
if (testHostExitedWithinTimeout == false)
Copy link
Member

Choose a reason for hiding this comment

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

The variable is nullable so you cannot simply use not.

@@ -163,6 +163,10 @@ This test may, or may not be the source of the crash.</value>
<data name="Seconds" xml:space="preserve">
<value>seconds</value>
</data>
<data name="Terminatin" xml:space="preserve">
Copy link
Member

Choose a reason for hiding this comment

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

Seems unused so far. Also, the resource key is using ing but the description is using ed we may want to synchronize that.

@Evangelink
Copy link
Member

@nohwnd Is there anything specific you want to test/handle before turning this into mergeable PR?

@MarcoRossignoli
Copy link
Contributor

:shipit:

Evangelink and others added 3 commits February 12, 2022 17:12
# Conflicts:
#	src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyOperationManager.cs
#	src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcDumpArgsBuilder.cs
@Evangelink
Copy link
Member

Closed in favor of #3466

@Evangelink Evangelink closed this Mar 14, 2022
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