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

Unblock run when cancellation was requested #5267

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 3 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
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,11 @@ private async Task ExecuteTestsInSourceAsync(IEnumerable<TestCase> tests, IRunCo

try
{
await Task.WhenAll(tasks);
// Ensure we can abandon currently running tasks on cancellation, rather than waiting for them to complete.
// They will still run on background but we won't await them.
var runCancelled = new TaskCompletionSource<object>();
_testRunCancellationToken?.Register(() => runCancelled.TrySetCanceled());
await Task.WhenAny(Task.WhenAll(tasks), runCancelled.Task);
Copy link
Member Author

Choose a reason for hiding this comment

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

Cancellation token _testRunCancellationToken is not a CancellationToken, so we cannot use Task.WhenAll(tasks, ct) here.

}
catch (Exception ex)
{
Expand All @@ -485,11 +489,13 @@ private async Task ExecuteTestsInSourceAsync(IEnumerable<TestCase> tests, IRunCo
// Queue the non parallel set
if (nonParallelizableTestSet != null)
{
// What to do?
await ExecuteTestsWithTestRunnerAsync(nonParallelizableTestSet, frameworkHandle, source, sourceLevelParameters, testRunner, usesAppDomains);
}
}
else
{
// What to do?
Copy link
Member Author

Choose a reason for hiding this comment

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

Here and few lines above we run the tests when parallelization is not enabled or test is marked with do not parallelize, this blocks the cancellation until the test finishes (and fails the non-parallel tests), and advice here how elegantly solve this? (it won't block if we have parallelism with 1 worker, because that takes the other parallel path.

Copy link
Member

Choose a reason for hiding this comment

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

@nohwnd How does it block cancellation? ExecuteTestsWithTestRunnerAsync calls ThrowIfCancellationRequested

Copy link
Member Author

@nohwnd nohwnd Mar 19, 2025

Choose a reason for hiding this comment

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

Yes but only before it starts running the test. if we cancel while the test is running we don't have a way to abandon it to background and continue to test run teardown code. Like writing test summary to screen. We still have 1 tests in progress and wait for it to complete.

Copy link
Member

Choose a reason for hiding this comment

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

You can still do the same Task.WhenAny trick right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah 😀 I must be tired

await ExecuteTestsWithTestRunnerAsync(testsToRun, frameworkHandle, source, sourceLevelParameters, testRunner, usesAppDomains);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,19 @@

[TestMethod]
[DynamicData(nameof(TargetFrameworks.AllForDynamicData), typeof(TargetFrameworks))]
public async Task AbortWithCTRLPlusC_CancellingTests(string tfm)
public async Task AbortWithCTRLPlusC_CancellingParallelTests(string tfm)
{
await AbortWithCTRLPlusC_CancellingTests(tfm, parallelize: true);

Check failure on line 19 in test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs

View check run for this annotation

Azure Pipelines / microsoft.testfx (Build MacOS Release)

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs#L19

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs(19,9): error IDE0022: (NETCORE_ENGINEERING_TELEMETRY=Build) Use expression body for method (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0022)

Check failure on line 19 in test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs

View check run for this annotation

Azure Pipelines / microsoft.testfx (Build MacOS Debug)

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs#L19

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs(19,9): error IDE0022: (NETCORE_ENGINEERING_TELEMETRY=Build) Use expression body for method (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0022)

Check failure on line 19 in test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs

View check run for this annotation

Azure Pipelines / microsoft.testfx (Build Linux Debug)

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs#L19

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs(19,9): error IDE0022: (NETCORE_ENGINEERING_TELEMETRY=Build) Use expression body for method (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0022)

Check failure on line 19 in test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs

View check run for this annotation

Azure Pipelines / microsoft.testfx (Build Linux Release)

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs#L19

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs(19,9): error IDE0022: (NETCORE_ENGINEERING_TELEMETRY=Build) Use expression body for method (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0022)

Check failure on line 19 in test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs

View check run for this annotation

Azure Pipelines / microsoft.testfx

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs#L19

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs(19,9): error IDE0022: (NETCORE_ENGINEERING_TELEMETRY=Build) Use expression body for method (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0022)
}

[TestMethod]
[DynamicData(nameof(TargetFrameworks.AllForDynamicData), typeof(TargetFrameworks))]
public async Task AbortWithCTRLPlusC_CancellingNonParallelTests(string tfm)
{
await AbortWithCTRLPlusC_CancellingTests(tfm, parallelize: false);

Check failure on line 26 in test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs

View check run for this annotation

Azure Pipelines / microsoft.testfx (Build MacOS Release)

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs#L26

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs(26,9): error IDE0022: (NETCORE_ENGINEERING_TELEMETRY=Build) Use expression body for method (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0022)

Check failure on line 26 in test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs

View check run for this annotation

Azure Pipelines / microsoft.testfx (Build MacOS Debug)

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs#L26

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs(26,9): error IDE0022: (NETCORE_ENGINEERING_TELEMETRY=Build) Use expression body for method (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0022)

Check failure on line 26 in test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs

View check run for this annotation

Azure Pipelines / microsoft.testfx (Build Linux Debug)

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs#L26

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs(26,9): error IDE0022: (NETCORE_ENGINEERING_TELEMETRY=Build) Use expression body for method (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0022)

Check failure on line 26 in test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs

View check run for this annotation

Azure Pipelines / microsoft.testfx (Build Linux Release)

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs#L26

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs(26,9): error IDE0022: (NETCORE_ENGINEERING_TELEMETRY=Build) Use expression body for method (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0022)

Check failure on line 26 in test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs

View check run for this annotation

Azure Pipelines / microsoft.testfx

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs#L26

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs(26,9): error IDE0022: (NETCORE_ENGINEERING_TELEMETRY=Build) Use expression body for method (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0022)
}

internal async Task AbortWithCTRLPlusC_CancellingTests(string tfm, bool parallelize)
{
// We expect the same semantic for Linux, the test setup is not cross and we're using specific
// Windows API because this gesture is not easy xplat.
Expand All @@ -25,17 +37,45 @@

var testHost = TestHost.LocateFrom(AssetFixture.TargetAssetPath, AssetName, tfm);

string? parameters = null;
if (parallelize)
{
// Providing runSettings even with Parallelize Workers = 1, will "enable" parallelization and will run via different path.
// So providing the settings only to the parallel run.
string runSettingsPath = Path.Combine(testHost.DirectoryName, $"{(parallelize ? "parallel" : "serial")}.runsettings");
File.WriteAllText(runSettingsPath, $"""
<RunSettings>
<MSTest>
<Parallelize>
<Workers>{(parallelize ? 0 : 1)}</Workers>
<Scope>MethodLevel</Scope>
</Parallelize>
</MSTest>
</RunSettings>
""");
parameters = $"--settings {runSettingsPath}";
}

string fileCreationPath = Path.Combine(testHost.DirectoryName, "fileCreation");
File.WriteAllText(fileCreationPath, string.Empty);
Copy link
Member Author

Choose a reason for hiding this comment

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

This was unblocking the loop below, so as soon as we started the app we cancelled tests before any test ever had a chance to run.

So this test was testing almost nothing.


TestHostResult testHostResult = await testHost.ExecuteAsync(environmentVariables: new()
TestHostResult testHostResult = await testHost.ExecuteAsync(parameters, environmentVariables: new()
{
["FILE_DIRECTORY"] = fileCreationPath,
});

testHostResult.AssertExitCodeIs(ExitCodes.TestSessionAborted);
// To ensure we don't cancel right away, so tests have chance to run, and block our
// cancellation if we do it wrong.
testHostResult.AssertOutputContains("Waiting for file creation.");
if (parallelize)
{
testHostResult.AssertOutputContains("Test Parallelization enabled for");
}
else
{
testHostResult.AssertOutputDoesNotContain("Test Parallelization enabled for");
}

testHostResult.AssertOutputMatchesRegex("Canceling the test session.*");
Copy link
Member Author

Choose a reason for hiding this comment

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

Now we abort so fast that this does not have a chance to write to screen.

testHostResult.AssertExitCodeIs(ExitCodes.TestSessionAborted);
}

public sealed class TestAssetFixture() : TestAssetFixtureBase(AcceptanceFixture.NuGetGlobalPackagesFolder)
Expand Down Expand Up @@ -92,6 +132,7 @@
}
else
{
Console.WriteLine("Waiting for file creation.");
Thread.Sleep(1000);
}
}
Expand Down Expand Up @@ -131,12 +172,18 @@
var fireCtrlCTask = Task.Run(() =>
{
// Delay for a short period before firing CTRL+C to simulate some processing time
Task.Delay(1000).Wait();
File.WriteAllText(Environment.GetEnvironmentVariable("FILE_DIRECTORY")!, string.Empty);
});

// Start a task that represents the infinite delay, which should be canceled
await Task.Delay(Timeout.Infinite, TestContext.CancellationTokenSource.Token);
// Wait for 10s, and after that kill the process.
// When we cancel by CRTL+C we do non-graceful teardown so the Environment.Exit should never be reached,
// because the test process already terminated.
//
// If we do reach it, we will see 11111 exit code, and it will fail the test assertion, because we did not cancel.
// (If we don't exit here, the process will happily run to completion after 10 seconds, but will still report
// cancelled exit code, so that is why we are more aggressive here.)
await Task.Delay(10_000);
Environment.Exit(11111);
}
}
""";
Expand Down
Loading