-
Notifications
You must be signed in to change notification settings - Fork 728
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
[RFC] Add extensibility to the async pipeline to allow custom implementation of waiting strategy #3037
base: main
Are you sure you want to change the base?
Conversation
#if ASYNC | ||
/// <summary> | ||
/// Implement this interface to customize the waiting behavior | ||
/// of the test adapter for <c>async</c> test methods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Test adapter" has a very specific meaning in the NUnit world. It's something outside the framework that adapts the representation of tests in NUnit to some other form. The most obvious example is the VS Test Adapter.
In this context (i.e. within the framework itself) it has no particular meaning for me. Sorry not to suggest an alternative, but I don't know what you are trying to say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking down farther, I see somebody has added an AwaitAdapter
in the past... I still don't like the term but I see that you aren't the one who introduced it. 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, "adapter" there refers to AwaitAdapter
and AsyncToSyncAdapter
. I can change to something else if the wording is troublesome.
Thanks @garuma. This PR is a great proof of concept because it shows that it doesn't take much code at all to get something basic working. I apologize for letting the time slip by. I want to work on this in two phases:
I'm going to comment in each of those places, for now. |
@garuma More on #2917 (comment). I would like to avoid having this ever be necessary for an NUnit extender to implement: // Custom wait strategy
[System.AttributeUsage (AttributeTargets.Class, Inherited = true, AllowMultiple = false)]
public class XwtAsyncWaitStrategyAttribute : Attribute, IAsyncCompletionStrategy
{
public void WaitForCompletion (IAwaiterObject awaitable)
{
while (!awaitable.IsCompleted)
Xwt.Application.MainLoop.DispatchPendingEvents ();
}
} Instead, you'd modify your first code sample to do something like this (I want to come up with an API to help make this simpler): // Run wrapper
[System.AttributeUsage(AttributeTargets.Class, Inherited = true, AllowMultiple = false)]
public sealed class XwtCommandWrapperAttribute : Attribute, IWrapSetUpTearDown
{
public TestCommand Wrap (TestCommand command) => new RunOnUICommand (command);
private sealed class RunOnUICommand : DelegatingTestCommand
{
public RunOnUICommand(TestCommand innerCommand)
: base (innerCommand)
{
}
public override TestResult Execute(TestExecutionContext context)
{
TestResult result = null;
Xwt.Application.Invoke(() =>
{
try
{
result = innerCommand.Execute(context);
}
catch (Exception e)
{
Console.WriteLine("Catastrophic fail", e);
}
Xwt.Application.Exit();
});
Xwt.Application.Run();
return result;
}
}
} This avoids the anti-pattern of calling Rather, the underlying framework should be using MsgWaitForMultipleObjectsEx or similar so that no CPU is consumed until an event occurs such as an event loop message or a synchronization context post. |
…n of waiting strategy.
79d0a70
to
7c8d7ef
Compare
(moved my comment to #2917 (comment)) |
@jnm2 What is the state of this now, and the corresponding issues? This is now 5 years old, but it may still have merit, right? |
The problem is that a DoEvents loop doesn't behave the same as an Application.Run() call, with a queued action and queued Exit() following the action. I wanted the API to be a different shape so that people don't go down the DoEvents loop route. |
@jnm2 Do you believe this can be made to work, or should this PR be abandoned ? It is now 5 years old...... |
This seems like a reason for concern...
|
/cc @jnm2
Related to:
This PR is an example of extensibility mechanism for the async machinery allowing external people to customize its behavior via attributes.
By leveraging the existing
IWrapSetUpTearDown
interface, one can now fully execute unit-test inside any sort of mainloop (GUI-based or not).For instance here is the set of custom attribute created to support running tests with XWT:
Additionally, this PR adds a
GetCombinedCustomAttributes
method to support looking up attribute definitions on both the test method and the containing fixture so that those two attributes above can be applied at the fixture level thus ensuring all the tests automatically inherit the behavior.(Note that this way of doing things work for our own test suite with minimal changes to the nunit codebase but I don't have any strong feeling about going in that specific direction)