Increase timeouts for datacollector to connect to Testhost #1406
Conversation
mayankbansal018
requested review from
acesiddhu,
smadala,
cltshivash,
singhsarab and
abhishkk
| @@ -109,7 +111,9 @@ public void GetFrameWorkForDotNetAssembly(string framework) | |||
| } | |||
|
|
|||
| Console.WriteLine("Framework:{0}, {1}", framework, string.Format(PerfAssertMessageFormat, expectedElapsedTime, stopWatch.ElapsedMilliseconds)); | |||
| Assert.IsTrue(stopWatch.ElapsedMilliseconds < expectedElapsedTime, string.Format(PerfAssertMessageFormat, expectedElapsedTime, stopWatch.ElapsedMilliseconds)); | |||
|
|
|||
There was a problem hiding this comment.
if we remove asserts what are we testing?
There was a problem hiding this comment.
What these tests are testing, is that we are correctly determining the architecture of dll, see lines above. Putting Assert that they should pass in within certain time is highly in-deterministic. Whether or not a test will complete in 5ms depends upon a lot of factors, like machine state, CPU cycles the test gets, etc..
Putting Assert around timeout is not a way to test performance, for that we should have an appropriate test-bed, along with a standard physical machine to benchmark things on
There was a problem hiding this comment.
| @@ -32,6 +32,8 @@ internal class DefaultEngineInvoker : | |||
| /// </summary> | |||
| private const int ClientListenTimeOut = Timeout.Infinite; | |||
There was a problem hiding this comment.
where is ClientListenTimeout used ? And can it be removed ?
There was a problem hiding this comment.
ClientListenTimeout is used by testhost to connect to vstest.console. We are waiting infinitely here so as to allow testhost to keep on polling vstest for comm. However vstest.console only waits for 60secs as default(this is configurable) to connect to testhost. If this does not connect to testhost in this time, it will itself kill testhost process. So this timeout will not cause deadlock
There was a problem hiding this comment.
Let us not have infinite timeouts anywhere in the code.. keep the timeout then the twice of the vstest.console.exe timeout.. but not infinite..
In reply to: 165572248 [](ancestors = 165572248)
There was a problem hiding this comment.
Like I said vstest.console will kill testhost process if cannot connect to it. So keeping an infinite timeout here is not harmful. Also the timeout at vstest.console is configurable, & not here. What I can do however is make this also configurable using same environment variable as we use for vstest.console, but then again allowing testhost to wait indefinitely to connect to vstest.console is not harfmful.
Let me know if I still need to make this change
| private const int DATACOLLECTIONCONNTIMEOUT = 15 * 1000; | ||
| // On Slower Machines(hosted agents) with Profiling enabled 15secs is not enough for testhost to get started(weird right!!), | ||
| // hence increasing this timeout | ||
| private const int DATACOLLECTIONCONNTIMEOUT = 60 * 1000; |
There was a problem hiding this comment.
DATACOLLECTIONCONNTIMEOUT [](start = 26, length = 25)
nitpick: can it be properly cased ? #Resolved
| // if somehow(on slower machines, with Profiler Enabled) testhost can take considerable time to launch, | ||
| // in such scenario dc.exe would have killed the server, but testhost will wait infinitely to connect to it, | ||
| // hence do not wait to connect to datacollector process infinitely, as it will cause process hang. | ||
| dataCollectionTestCaseEventSender.WaitForRequestSenderConnection(DataConnectionClientListenTimeOut); |
There was a problem hiding this comment.
WaitForRequestSenderConnection [](start = 54, length = 30)
if we fail to connect we aren't logging anything .. minimal we should do that.. also what will happen when timeout kicks in,,,?
There was a problem hiding this comment.
Logging done. If timeout kicks in vstest will only run test, & data collection will fail
| Assert.AreEqual(Enum.Parse(typeof(Architecture), platform, ignoreCase: true), arch); | ||
|
|
||
| // We should not assert on time elapsed, it will vary depending on machine, & their state, commenting below assert | ||
| // Assert.IsTrue(stopWatch.ElapsedMilliseconds < expectedElapsedTime, string.Format(PerfAssertMessageFormat, expectedElapsedTime, stopWatch.ElapsedMilliseconds)); |
There was a problem hiding this comment.
can we remove it then ? #WontFix
There was a problem hiding this comment.
I don't want to remove this commented out as:
- It is in test code so I can live with it.
- If I had removed it to being with, I would not have been able to explain why I did so in first place.
- It's kind of a reminder that do not add perf test in Unit Tests #WontFix
|
@dotnet-bot Windows_NT / Debug Build |
|
@dotnet-bot test Windows_NT / Debug Build |
Description
On Slower Machines(hosted agents) with Profiling enabled 15secs is not enough for testhost to get started,
hence with data collection enabled, the connection b/w testhost, & dc.exe fails.
TestHost process currently waits infinitely to connect to dc.exe, if the above connection does not happen, it causes a hang in th.exe
Fixes #1410