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.

Sign up for GitHub

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

Cancellation2 #5288

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Cancellation2 #5288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
024c3d8
Unblock run when cancellation was requested but there are still runni…
nohwnd Mar 19, 2025
0e7051c
Fix test to test cancellation correctly
nohwnd Mar 19, 2025
a4afd44
what to do ?
nohwnd Mar 19, 2025
c422b39
Will continue later
nohwnd Mar 19, 2025
77355ac
Yield so we can abandon
nohwnd Mar 20, 2025
3ed9e8f
Yield so we can abandon
nohwnd Mar 20, 2025
f90b912
better way
nohwnd Mar 21, 2025
1f6e707
remove
nohwnd Mar 21, 2025
f50e794
Merge branch 'main' into cancellation2
nohwnd Mar 21, 2025
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
2 changes: 1 addition & 1 deletion samples/Playground/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public static async Task<int> Main(string[] args)
{
#if NETCOREAPP
// To attach to the children
Microsoft.Testing.TestInfrastructure.DebuggerUtility.AttachCurrentProcessToParentVSProcess();
// Microsoft.Testing.TestInfrastructure.DebuggerUtility.AttachCurrentProcessToParentVSProcess();
#endif

ITestApplicationBuilder testApplicationBuilder = await TestApplication.CreateBuilderAsync(args);
Expand Down
6 changes: 5 additions & 1 deletion src/Platform/Microsoft.Testing.Platform/Requests/TestHostTestFrameworkInvoker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@ public async Task ExecuteAsync(ITestFramework testFramework, ClientInfo client,
IMessageBus messageBus = ServiceProvider.GetMessageBus();

// Execute the test request
await ExecuteRequestAsync(testFramework, request, messageBus, cancellationToken);
var taskCompletionSource = new TaskCompletionSource();
using CancellationTokenRegistration r = cancellationToken.Register(() => taskCompletionSource.TrySetCanceled());
await Task.WhenAny(ExecuteRequestAsync(testFramework, request, messageBus, cancellationToken), taskCompletionSource.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.

Here we ensure that the request run can be abandoned without completing when canelation token is cancelled (this token is test session cancellation token).

This will give the test framework a chance to run CloseTestSessionAsync https://github.com/microsoft/testfx/blob/main/src/Platform/Microsoft.Testing.Platform/TestFramework/ITestFramework.cs

which seems to align with the intent of the interface and the token.

Copy link
Member

Choose a reason for hiding this comment

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

That makes it pointless to pass a cancellation token to framework authors right? Because now we will force-cancel on our side.

I'd expect framework authors to handle it. So, if we want to change something, I'd change in MSTest rather than MTP

Copy link
Member

Choose a reason for hiding this comment

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

And thinking more, this is too risky. If a cancellation is requested, we may be proceeding to CloseTestSessionAsync while ExecuteRequestAsync is still running right? We just bypassed it by the WhenAny but it's still there and running

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes it pointless to pass a cancellation token to framework authors right?

I don't think it is pointless. We pass the same token within the whole test session via Services.TestSessionContext. This cancellation token is called abortRun.

The test framework has 3 callbacks, before test session, execute request, and after test session.

In TestHostFrameworkInvoker we pass the abortTestRun to all those 3 callbacks. And before each operation we check if we should do more work, or if cancellation was requested.

If it was requested, we abort, if not we continue.

Problem is when long running work starts in the testFramework.ExecuteRequestAsync, we don't have a way out, which is what this PR implements.

When executeRequestAsync is running long synchronous work, we cancel the token, we will ignore that running work and go directly to test session teardown that is provided by the framework.

Once the single item of work finishes in testframework.executerequestasync, no more work should be started because framework author should check for cancellation before starting any work. This is very similar to what we do internally in mstest when timeout for test is specified and the test times out (IMHO).

The risky point is what this PR is trying to achieve, so we don't have to rely on framework authors to not block the platform from completing. This is non-graceful teardown of the framework.

To handle this predictably, there needs to be a whole other layer (or two).

The testframework.ExecuteRequest should get a separate cancelation token that will cancel the run, and there should be a different gesture for Cancel of test run (which is more graceful) and Abort (which is less graceful but still not as aggressive as killing the process), which is what we now call Cancel.


CloseTestSessionResult closeTestSessionResult = await testFramework.CloseTestSessionAsync(new(sessionId, client, cancellationToken));
await HandleTestSessionResultAsync(closeTestSessionResult.IsSuccess, closeTestSessionResult.WarningMessage, closeTestSessionResult.ErrorMessage);
Expand All @@ -68,6 +70,8 @@ public async Task ExecuteAsync(ITestFramework testFramework, ClientInfo client,

public virtual async Task ExecuteRequestAsync(ITestFramework testFramework, TestExecutionRequest request, IMessageBus messageBus, CancellationToken cancellationToken)
{
await Task.Yield();
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 we ensure that this method will run asynchronously, so we can handle the Task.WhenAny above in that call stack.


using SemaphoreSlim requestSemaphore = new(0, 1);
await testFramework.ExecuteRequestAsync(new(request, messageBus, new SemaphoreSlimRequestCompleteNotifier(requestSemaphore), cancellationToken));
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why the semaphore slim is needed here, instead of just completing and awaiting the task. The semaphore is passed in via context and test frameworks are supposed to call Complete on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/thomhurst/TUnit/blob/main/TUnit.Engine/Framework/TUnitTestFramework.cs#L170 this code does not do it, but in theory the ExecuteRequestAsync can complete and pass the context to something else, which will then call Complete when done.

Seems rather equivalent to just awaiting the task. Because we will still await some completion.

Copy link
Member

Choose a reason for hiding this comment

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

Unless there is something I don't see, I think it could be reasonable to consider changing that, but probably in v2 as Complete is public already. So we can remove Complete and rely on the returned task. If framework authors want to "complete" on another thread or whatever, they could create TCS and set result in whatever place they would normally call "Complete" in. That's separate from cancellation though

await requestSemaphore.WaitAsync(cancellationToken);
Expand Down
59 changes: 51 additions & 8 deletions test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AbortionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,15 @@ public sealed class AbortionTests : AcceptanceTestBase<AbortionTests.TestAssetFi

[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);

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

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 +33,45 @@ public async Task AbortWithCTRLPlusC_CancellingTests(string tfm)

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);

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.*");
testHostResult.AssertExitCodeIs(ExitCodes.TestSessionAborted);
}

public sealed class TestAssetFixture() : TestAssetFixtureBase(AcceptanceFixture.NuGetGlobalPackagesFolder)
Expand Down Expand Up @@ -92,6 +128,7 @@ public static async Task<int> Main(string[] args)
}
else
{
Console.WriteLine("Waiting for file creation.");
Thread.Sleep(1000);
}
}
Expand Down Expand Up @@ -131,12 +168,18 @@ public async Task TestA()
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