Skip to content
Open
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 58 additions & 46 deletions src/Utilities.UnitTests/ToolTask_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Diagnostics;
using System.Globalization;
using System.IO;
using System.Resources;
using System.Text.RegularExpressions;
Expand All @@ -20,7 +21,7 @@ namespace Microsoft.Build.UnitTests
{
public sealed class ToolTask_Tests
{
private ITestOutputHelper _output;
private readonly ITestOutputHelper _output;

public ToolTask_Tests(ITestOutputHelper testOutput)
{
Expand Down Expand Up @@ -993,50 +994,61 @@ public void SetsTerminationTimeoutCorrectly(int timeout, bool isInvalidValid)
/// Verifies that a ToolTask instance can return correct results when executed multiple times with timeout.
/// </summary>
/// <param name="repeats">Specifies the number of repeats for external command execution.</param>
/// <param name="initialDelay">Delay to generate on the first execution in milliseconds.</param>
/// <param name="followupDelay">Delay to generate on follow-up execution in milliseconds.</param>
/// <param name="timeout">Task timeout in milliseconds.</param>
/// <param name="timeoutOnFirstExecution">Whether the first execution should be forced to time out before later retries succeed.</param>
/// <remarks>
/// These tests execute the same task instance multiple times, which will in turn run a shell command to sleep
/// predefined amount of time. The first execution may time out, but all following ones won't. It is expected
/// that all following executions return success.
/// These tests execute the same task instance multiple times using a direct OS sleep process rather than
/// shelling through PowerShell or cmd.exe. When configured to time out, the first execution is expected to
/// fail and all following executions must still succeed.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Suggested change
/// These tests execute the same task instance multiple times using a direct OS sleep process rather than
/// shelling through PowerShell or cmd.exe. When configured to time out, the first execution is expected to
/// fail and all following executions must still succeed.
/// These tests execute the same task instance multiple times, which will in turn run a command to sleep for a
/// predefined amount of time. The first execution may time out, but all following ones won't. It is expected
/// that all following executions return success.```

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.

Applied the suggested text in ac443a9.

/// </remarks>
[Theory]
[InlineData(1, 1, 1, -1)] // Normal case, no repeat.
[InlineData(3, 1, 1, -1)] // Repeat without timeout.
[InlineData(3, 10000, 1, 1000)] // Repeat with timeout.
public void ToolTaskThatTimeoutAndRetry(int repeats, int initialDelay, int followupDelay, int timeout)
[InlineData(1, false)]
[InlineData(3, false)]
[InlineData(3, true)]
public void ToolTaskThatTimeoutAndRetry(int repeats, bool timeoutOnFirstExecution)
{
using var env = TestEnvironment.Create(_output);

int fastDelayMilliseconds = NativeMethodsShared.IsWindows ? 1_000 : 100;
int slowDelayMilliseconds = 5_000;
int timeoutMilliseconds = NativeMethodsShared.IsWindows ? 2_000 : 1_000;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@copilot does this need to vary? Shouldn't it be irrelevant for the fast delay, and so could be 2s in both OS cases?

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.

No, it doesn't need to vary. Removed the OS conditional — timeoutMilliseconds is now a flat 2_000 on all platforms (ac443a9). fastDelayMilliseconds is left as-is per the follow-up instruction.


MockEngine3 engine = new();

// Task under test:
var task = new ToolTaskThatSleeps
{
BuildEngine = engine,
InitialDelay = initialDelay,
FollowupDelay = followupDelay,
Timeout = timeout
InitialDelay = timeoutOnFirstExecution ? slowDelayMilliseconds : fastDelayMilliseconds,
FollowupDelay = fastDelayMilliseconds,
Timeout = timeoutOnFirstExecution ? timeoutMilliseconds : System.Threading.Timeout.Infinite
};

// Execute the same task instance multiple times. The index is one-based.
bool result;
for (int i = 1; i <= repeats; i++)
for (int attempt = 1; attempt <= repeats; attempt++)
{
// Execute the task:
result = task.Execute();
bool shouldSucceed = attempt > 1 || !timeoutOnFirstExecution;
bool result = task.Execute();

_output.WriteLine(engine.Log);
_output.WriteLine(
$"Attempt {attempt}/{repeats}: expectedSuccess={shouldSucceed}, actualSuccess={result}, exitCode={task.ExitCode}.");

if (!string.IsNullOrEmpty(engine.Log))
{
_output.WriteLine(engine.Log);
engine.Log = string.Empty;
}

task.RepeatCount.ShouldBe(i);
task.RepeatCount.ShouldBe(attempt);
result.ShouldBe(shouldSucceed);

// The first execution may fail (timeout), but all following ones should succeed:
if (i > 1)
if (shouldSucceed)
{
result.ShouldBeTrue();
task.ExitCode.ShouldBe(0);
}
else
{
task.ExitCode.ShouldNotBe(0);
}
}
}

Expand Down Expand Up @@ -1106,25 +1118,21 @@ public void ToolTaskCapturesAllOutputWithFix()
/// A simple implementation of <see cref="ToolTask"/> to sleep for a while.
/// </summary>
/// <remarks>
/// This task runs shell command to sleep for predefined, variable amount of time based on how many times the
/// instance has been executed.
/// This task invokes a direct sleep tool with a variable delay based on how many times the instance has been
/// executed, which avoids the flakiness of nesting the wait inside a shell.
/// </remarks>
private sealed class ToolTaskThatSleeps : ToolTask
{
// Windows prompt command to sleep:
private readonly string _windowsSleep = "/c start /wait timeout {0}";
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This start /wait was (at least part of!) the reason running tests in our repo popped up terminal windows over and over. Not sure if that was bugging anybody else but it drove me up the wall!


// UNIX command to sleep:
private readonly string _unixSleep = "-c \"sleep {0}\"";

// Full path to shell:
private readonly string _pathToShell;
private readonly string _pathToTool;

public ToolTaskThatSleeps()
: base()
{
// Determines shell to use: cmd for Windows, sh for UNIX-like systems:
_pathToShell = NativeMethodsShared.IsUnixLike ? "/bin/sh" : "cmd.exe";
// timeout.exe exits immediately when ToolTask redirects stdin on Windows, so use ping.exe
// as the built-in blocking process for the timeout/retry scenario.
_pathToTool = NativeMethodsShared.IsUnixLike
? "sleep"
: Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.System), "ping.exe");
}

/// <summary>
Expand All @@ -1139,9 +1147,9 @@ public ToolTaskThatSleeps()
/// Gets or sets the delay for the follow-up executions.
/// </summary>
/// <remarks>
/// Defaults to 1 milliseconds.
/// Defaults to 100 milliseconds.
/// </remarks>
public Int32 FollowupDelay { get; set; } = 1;
public Int32 FollowupDelay { get; set; } = 100;

/// <summary>
/// Int32 output parameter for the repeat counter for test purpose.
Expand All @@ -1150,22 +1158,26 @@ public ToolTaskThatSleeps()
public Int32 RepeatCount { get; private set; } = 0;

/// <summary>
/// Gets the tool name (shell).
/// Gets the tool name.
/// </summary>
protected override string ToolName => Path.GetFileName(_pathToShell);
protected override string ToolName => Path.GetFileName(_pathToTool);

/// <summary>
/// Gets the full path to shell.
/// Gets the full path to the sleep tool.
/// </summary>
protected override string GenerateFullPathToTool() => _pathToShell;
protected override string GenerateFullPathToTool() => _pathToTool;

/// <summary>
/// Generates a shell command to sleep different amount of time based on repeat counter.
/// Generates the arguments to sleep for a different amount of time based on repeat counter.
/// </summary>
protected override string GenerateCommandLineCommands() =>
NativeMethodsShared.IsUnixLike ?
string.Format(_unixSleep, RepeatCount < 2 ? InitialDelay / 1000.0 : FollowupDelay / 1000.0) :
string.Format(_windowsSleep, RepeatCount < 2 ? InitialDelay / 1000.0 : FollowupDelay / 1000.0);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hilariously, before this change we tried to timeout 0.001 and it worked only by

  • returning immediately
  • but we ignored the error code.

protected override string GenerateCommandLineCommands()
{
int delay = RepeatCount < 2 ? InitialDelay : FollowupDelay;

return NativeMethodsShared.IsUnixLike
? (delay / 1000.0).ToString("0.###", CultureInfo.InvariantCulture)
: $"-n {Math.Max(2, (int)Math.Ceiling(delay / 1000.0) + 1).ToString(CultureInfo.InvariantCulture)} 127.0.0.1";
}

/// <summary>
/// Ensures that test parameters make sense.
Expand Down
Loading