[Bug] [Flaky test]: TestBuildWithChildren unit test is unreliable and may fail on an apparent race condition

[jrdodds]

Issue Description

The Microsoft.Build.UnitTests.BackEnd.BuildRequestEngine_Tests.TestBuildWithChildren unit test failed in a PR build with an the following assert:

Assert.Equal() Failure
Expected: Active
Actual: Idle

The location of the error is src\Build.UnitTests\BackEnd\BuildRequestEngine_Tests.cs:line 408

A subsequent build, different only by a change to a comment, succeeded.

Steps to Reproduce

Run the set of unit tests.

It appears to be a threading race condition and the test doesn't always break.

Expected Behavior

The unit test should be reliable and test what it is intended to test.

Actual Behavior

The unit test may fail while creating the conditions for the test.

Analysis

msbuild/src/Build.UnitTests/BackEnd/BuildRequestEngine_Tests.cs

Lines 399 to 409 in 0bc6323

	// Create the initial build request
	string[] targets = new string[3] { "target1", "target2", "target3" };
	BuildRequest request = CreateNewBuildRequest(1, targets);
	// Kick it off
	VerifyEngineStatus(BuildRequestEngineStatus.Uninitialized);
	_engine.InitializeForBuild(new NodeLoggingContext(_host.LoggingService, 0, false));
	_engine.SubmitBuildRequest(request);
	Thread.Sleep(250);
	VerifyEngineStatus(BuildRequestEngineStatus.Active);

The code assumes that after the Thread.Sleep(250) at line 407, the engine status will be Active. Instead, the engine status may be Idle. There is a second instance of calling Thread.Sleep() and VerifyEngineStatus() later in the test method.

Sleeping for an arbitrary time and then assuming a certain state is inherently risky. Can this test be implemented without using Thread.Sleep()?

Versions & Configurations

MSBuild version 17.9.0-dev-23476-01+abb507c0a for .NET
17.9.0.47601

[JaynieBai]

Related with #9100 and fixing with PR #9215