From c2600cffc9888580c318c7bb05f3d8f4328b2e43 Mon Sep 17 00:00:00 2001 From: Stefan Holpp Date: Tue, 15 Apr 2025 08:40:47 +0200 Subject: [PATCH 1/2] Simplification of HookExtension by removing the _Invoke methods becasue they are not necessary and caused bugs. --- .../Internal/HookExtensions/AsyncEvent.cs | 25 +++---- .../Internal/HookExtensions/HookExtension.cs | 50 +++++++------- .../tests/HookExtension/ConstructorTests.cs | 65 +++++++++++++++++++ .../Internal/HookExtension/AsyncEventTests.cs | 9 ++- 4 files changed, 107 insertions(+), 42 deletions(-) create mode 100644 src/NUnitFramework/tests/HookExtension/ConstructorTests.cs diff --git a/src/NUnitFramework/framework/Internal/HookExtensions/AsyncEvent.cs b/src/NUnitFramework/framework/Internal/HookExtensions/AsyncEvent.cs index 582ccb2bc6..6af9c938f2 100644 --- a/src/NUnitFramework/framework/Internal/HookExtensions/AsyncEvent.cs +++ b/src/NUnitFramework/framework/Internal/HookExtensions/AsyncEvent.cs @@ -15,17 +15,6 @@ public sealed class AsyncEvent private readonly List _handlers = new(); private readonly List _asyncHandlers = new(); - /// - /// Constructor that initializes the event and provides an event handler to invoke it. - /// - /// - /// The event handler to invoke the event. - /// - public AsyncEvent(out AsyncEventHandler invoke) - { - invoke = Invoke; - } - /// /// Adds a synchronous handler to the event. /// @@ -46,7 +35,19 @@ public void AddAsyncHandler(AsyncEventHandler asyncHandler) _asyncHandlers.Add(asyncHandler); } - private Task Invoke(object? sender, TEventArgs e) + internal IReadOnlyList GetHandlers() + { + lock (_handlers) + return _handlers; + } + + internal IReadOnlyList GetAsyncHandlers() + { + lock (_handlers) + return _asyncHandlers; + } + + internal Task Invoke(object? sender, TEventArgs e) { if (!_handlers.Any() && !_asyncHandlers.Any()) { diff --git a/src/NUnitFramework/framework/Internal/HookExtensions/HookExtension.cs b/src/NUnitFramework/framework/Internal/HookExtensions/HookExtension.cs index 2fcac8d8f3..6a4491f4c0 100644 --- a/src/NUnitFramework/framework/Internal/HookExtensions/HookExtension.cs +++ b/src/NUnitFramework/framework/Internal/HookExtensions/HookExtension.cs @@ -17,21 +17,14 @@ public class HookExtension /// public HookExtension() { - BeforeAnySetUps = new AsyncEvent(out _invokeBeforeAnySetUps); - AfterAnySetUps = new AsyncEvent(out _invokeAfterAnySetUps); - BeforeTest = new AsyncEvent(out _invokeBeforeTest); - AfterTest = new AsyncEvent(out _invokeAfterTest); - BeforeAnyTearDowns = new AsyncEvent(out _invokeBeforeAnyTearDowns); - AfterAnyTearDowns = new AsyncEvent(out _invokeAfterAnyTearDowns); + BeforeAnySetUps = new AsyncEvent(); + AfterAnySetUps = new AsyncEvent(); + BeforeTest = new AsyncEvent(); + AfterTest = new AsyncEvent(); + BeforeAnyTearDowns = new AsyncEvent(); + AfterAnyTearDowns = new AsyncEvent(); } - private AsyncEventHandler _invokeBeforeAnySetUps; - private AsyncEventHandler _invokeAfterAnySetUps; - private AsyncEventHandler _invokeBeforeTest; - private AsyncEventHandler _invokeAfterTest; - private AsyncEventHandler _invokeBeforeAnyTearDowns; - private AsyncEventHandler _invokeAfterAnyTearDowns; - /// /// Gets or sets the hook event that is triggered before any setup methods are executed. /// @@ -68,42 +61,49 @@ public HookExtension() /// The instance of to copy hooks from. public HookExtension(HookExtension other) : this() { - other._invokeBeforeAnySetUps?.GetInvocationList()?.ToList().ForEach(d => _invokeBeforeAnySetUps += d as AsyncEventHandler); - other._invokeAfterAnySetUps?.GetInvocationList()?.ToList().ForEach(d => _invokeAfterAnySetUps += d as AsyncEventHandler); - other._invokeBeforeTest?.GetInvocationList()?.ToList().ForEach(d => _invokeBeforeTest += d as AsyncEventHandler); - other._invokeAfterTest?.GetInvocationList()?.ToList().ForEach(d => _invokeAfterTest += d as AsyncEventHandler); - other._invokeBeforeAnyTearDowns?.GetInvocationList()?.ToList().ForEach(d => _invokeBeforeAnyTearDowns += d as AsyncEventHandler); - other._invokeAfterAnyTearDowns?.GetInvocationList()?.ToList().ForEach(d => _invokeAfterAnyTearDowns += d as AsyncEventHandler); + other.BeforeAnySetUps.GetHandlers().ToList().ForEach(d => BeforeAnySetUps.AddHandler((EventHandler)d)); + other.AfterAnySetUps.GetHandlers().ToList().ForEach(d => AfterAnySetUps.AddHandler((EventHandler)d)); + other.BeforeTest.GetHandlers().ToList().ForEach(d => BeforeTest.AddHandler((EventHandler)d)); + other.AfterTest.GetHandlers().ToList().ForEach(d => AfterTest.AddHandler((EventHandler)d)); + other.BeforeAnyTearDowns.GetHandlers().ToList().ForEach(d => BeforeAnyTearDowns.AddHandler((EventHandler)d)); + other.AfterAnyTearDowns.GetHandlers().ToList().ForEach(d => AfterAnyTearDowns.AddHandler((EventHandler)d)); + + other.BeforeAnySetUps.GetAsyncHandlers().ToList().ForEach(d => BeforeAnySetUps.AddAsyncHandler((AsyncEventHandler)d)); + other.AfterAnySetUps.GetAsyncHandlers().ToList().ForEach(d => AfterAnySetUps.AddAsyncHandler((AsyncEventHandler)d)); + other.BeforeTest.GetAsyncHandlers().ToList().ForEach(d => BeforeTest.AddAsyncHandler((AsyncEventHandler)d)); + other.AfterTest.GetAsyncHandlers().ToList().ForEach(d => AfterTest.AddAsyncHandler((AsyncEventHandler)d)); + other.BeforeAnyTearDowns.GetAsyncHandlers().ToList().ForEach(d => BeforeAnyTearDowns.AddAsyncHandler((AsyncEventHandler)d)); + other.AfterAnyTearDowns.GetAsyncHandlers().ToList().ForEach(d => AfterAnyTearDowns.AddAsyncHandler((AsyncEventHandler)d)); } internal void OnBeforeAnySetUps(TestExecutionContext context, IMethodInfo method) { - _invokeBeforeAnySetUps(this, new TestHookIMethodEventArgs(context, method)); + await BeforeAnySetUps.Invoke(this, new TestHookIMethodEventArgs(context, method)); } internal void OnAfterAnySetUps(TestExecutionContext context, IMethodInfo method, Exception? exceptionContext = null) { - _invokeAfterAnySetUps(this, new TestHookIMethodEventArgs(context, method, exceptionContext)); + await AfterAnySetUps.Invoke(this, new TestHookIMethodEventArgs(context, method, exceptionContext)); } internal void OnBeforeTest(TestExecutionContext context) { - _invokeBeforeTest(this, new TestHookTestMethodEventArgs(context)); + await BeforeTest.Invoke(this, new TestHookTestMethodEventArgs(context)); } internal void OnAfterTest(TestExecutionContext context, Exception? exceptionContext = null) { - _invokeAfterTest(this, new TestHookTestMethodEventArgs(context, exceptionContext)); + await AfterTest.Invoke(this, new TestHookTestMethodEventArgs(context, exceptionContext)); } internal void OnBeforeAnyTearDowns(TestExecutionContext context, IMethodInfo method) { - _invokeBeforeAnyTearDowns(this, new TestHookIMethodEventArgs(context, method)); + await BeforeAnyTearDowns.Invoke(this, new TestHookIMethodEventArgs(context, method)); } internal void OnAfterAnyTearDowns(TestExecutionContext context, IMethodInfo method, Exception? exceptionContext = null) { - _invokeAfterAnyTearDowns(this, new TestHookIMethodEventArgs(context, method, exceptionContext)); + await AfterAnyTearDowns.Invoke(this, new TestHookIMethodEventArgs(context, method, exceptionContext)); } } diff --git a/src/NUnitFramework/tests/HookExtension/ConstructorTests.cs b/src/NUnitFramework/tests/HookExtension/ConstructorTests.cs new file mode 100644 index 0000000000..b1b43ec2f6 --- /dev/null +++ b/src/NUnitFramework/tests/HookExtension/ConstructorTests.cs @@ -0,0 +1,65 @@ +// Copyright (c) Charlie Poole, Rob Prouse and Contributors. MIT License - see LICENSE.txt + +namespace NUnit.Framework.Tests.HookExtension +{ + internal class HookExtensionConstructorTests + { + [Test] + public void CopyCtor_CreateNewHookExtension_InvocationListShouldBeEmpty() + { + var hookExt = new NUnit.Framework.Internal.HookExtensions.HookExtension(); + + Assert.Multiple(() => + { + Assert.That(hookExt.BeforeAnySetUps.GetHandlers().Count, Is.EqualTo(0)); + Assert.That(hookExt.AfterAnySetUps.GetHandlers().Count, Is.EqualTo(0)); + Assert.That(hookExt.BeforeTest.GetHandlers().Count, Is.EqualTo(0)); + Assert.That(hookExt.AfterTest.GetHandlers().Count, Is.EqualTo(0)); + Assert.That(hookExt.BeforeAnyTearDowns.GetHandlers().Count, Is.EqualTo(0)); + Assert.That(hookExt.AfterAnyTearDowns.GetHandlers().Count, Is.EqualTo(0)); + + Assert.That(hookExt.BeforeAnySetUps.GetAsyncHandlers().Count, Is.EqualTo(0)); + Assert.That(hookExt.AfterAnySetUps.GetAsyncHandlers().Count, Is.EqualTo(0)); + Assert.That(hookExt.BeforeTest.GetAsyncHandlers().Count, Is.EqualTo(0)); + Assert.That(hookExt.AfterTest.GetAsyncHandlers().Count, Is.EqualTo(0)); + Assert.That(hookExt.BeforeAnyTearDowns.GetAsyncHandlers().Count, Is.EqualTo(0)); + Assert.That(hookExt.AfterAnyTearDowns.GetAsyncHandlers().Count, Is.EqualTo(0)); + }); + } + + [Test] + public void CopyCtor_CallMultipleTimes_ShallNotIncreaseInvocationList() + { + var hookExt = new NUnit.Framework.Internal.HookExtensions.HookExtension(); + hookExt.AfterTest.AddHandler((sender, args) => { }); + hookExt.AfterTest.AddAsyncHandler(async (sender, args) => { }); + + hookExt = new NUnit.Framework.Internal.HookExtensions.HookExtension(hookExt); + hookExt = new NUnit.Framework.Internal.HookExtensions.HookExtension(hookExt); + hookExt = new NUnit.Framework.Internal.HookExtensions.HookExtension(hookExt); + + // initially assigned handlers shall be copied + Assert.Multiple(() => + { + Assert.That(hookExt.AfterTest.GetHandlers().Count, Is.EqualTo(1)); + Assert.That(hookExt.AfterTest.GetAsyncHandlers().Count, Is.EqualTo(1)); + }); + + // all others shall stay empty + Assert.Multiple(() => + { + Assert.That(hookExt.BeforeAnySetUps.GetHandlers().Count, Is.EqualTo(0)); + Assert.That(hookExt.AfterAnySetUps.GetHandlers().Count, Is.EqualTo(0)); + Assert.That(hookExt.BeforeTest.GetHandlers().Count, Is.EqualTo(0)); + Assert.That(hookExt.BeforeAnyTearDowns.GetHandlers().Count, Is.EqualTo(0)); + Assert.That(hookExt.AfterAnyTearDowns.GetHandlers().Count, Is.EqualTo(0)); + + Assert.That(hookExt.BeforeAnySetUps.GetAsyncHandlers().Count, Is.EqualTo(0)); + Assert.That(hookExt.AfterAnySetUps.GetAsyncHandlers().Count, Is.EqualTo(0)); + Assert.That(hookExt.BeforeTest.GetAsyncHandlers().Count, Is.EqualTo(0)); + Assert.That(hookExt.BeforeAnyTearDowns.GetAsyncHandlers().Count, Is.EqualTo(0)); + Assert.That(hookExt.AfterAnyTearDowns.GetAsyncHandlers().Count, Is.EqualTo(0)); + }); + } + } +} diff --git a/src/NUnitFramework/tests/Internal/HookExtension/AsyncEventTests.cs b/src/NUnitFramework/tests/Internal/HookExtension/AsyncEventTests.cs index f07643bce2..46ea9c08ae 100644 --- a/src/NUnitFramework/tests/Internal/HookExtension/AsyncEventTests.cs +++ b/src/NUnitFramework/tests/Internal/HookExtension/AsyncEventTests.cs @@ -10,22 +10,21 @@ public class AsyncEventTests [Test] public void Invoke_AsyncEventThrowingException_AggregatedExceptionIsThrown() { - var asyncEvent = new AsyncEvent(out var invoke); + var asyncEvent = new AsyncEvent(); var exception = new Exception("Test exception"); var testMethodEventArgs = new TestHookTestMethodEventArgs(null); asyncEvent.AddAsyncHandler(async (sender, args) => throw exception); - Assert.Throws(() => invoke(this, testMethodEventArgs).Wait()); + Assert.Throws(() => asyncEvent.Invoke(this, testMethodEventArgs).Wait()); } [Test] public void Invoke_SyncEventThrowingException_AggregatedExceptionIsThrown() { - var syncEvent = new AsyncEvent(out var invoke); + var syncEvent = new AsyncEvent(); var exception = new Exception("Test exception"); var testMethodEventArgs = new TestHookTestMethodEventArgs(null); syncEvent.AddHandler((sender, args) => throw exception); - Assert.Throws(() => invoke(this, testMethodEventArgs).Wait()); + Assert.Throws(() => syncEvent.Invoke(this, testMethodEventArgs).Wait()); } - } } From f0fe8eeb0c82292d64c7f61778e70af3287d383d Mon Sep 17 00:00:00 2001 From: Stefan Holpp Date: Tue, 15 Apr 2025 08:54:23 +0200 Subject: [PATCH 2/2] Made outer call of Hook delegates synchronous. This ensures exceptions are propagates correctly from the handlers to NUnit. Async handlers are still executed asynchronously inside the AsyncEvent. --- .../Internal/HookExtensions/HookExtension.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/NUnitFramework/framework/Internal/HookExtensions/HookExtension.cs b/src/NUnitFramework/framework/Internal/HookExtensions/HookExtension.cs index 6a4491f4c0..915b049880 100644 --- a/src/NUnitFramework/framework/Internal/HookExtensions/HookExtension.cs +++ b/src/NUnitFramework/framework/Internal/HookExtensions/HookExtension.cs @@ -78,32 +78,32 @@ public HookExtension(HookExtension other) : this() internal void OnBeforeAnySetUps(TestExecutionContext context, IMethodInfo method) { - await BeforeAnySetUps.Invoke(this, new TestHookIMethodEventArgs(context, method)); + BeforeAnySetUps.Invoke(this, new TestHookIMethodEventArgs(context, method)); } internal void OnAfterAnySetUps(TestExecutionContext context, IMethodInfo method, Exception? exceptionContext = null) { - await AfterAnySetUps.Invoke(this, new TestHookIMethodEventArgs(context, method, exceptionContext)); + AfterAnySetUps.Invoke(this, new TestHookIMethodEventArgs(context, method, exceptionContext)); } internal void OnBeforeTest(TestExecutionContext context) { - await BeforeTest.Invoke(this, new TestHookTestMethodEventArgs(context)); + BeforeTest.Invoke(this, new TestHookTestMethodEventArgs(context)); } internal void OnAfterTest(TestExecutionContext context, Exception? exceptionContext = null) { - await AfterTest.Invoke(this, new TestHookTestMethodEventArgs(context, exceptionContext)); + AfterTest.Invoke(this, new TestHookTestMethodEventArgs(context, exceptionContext)); } internal void OnBeforeAnyTearDowns(TestExecutionContext context, IMethodInfo method) { - await BeforeAnyTearDowns.Invoke(this, new TestHookIMethodEventArgs(context, method)); + BeforeAnyTearDowns.Invoke(this, new TestHookIMethodEventArgs(context, method)); } internal void OnAfterAnyTearDowns(TestExecutionContext context, IMethodInfo method, Exception? exceptionContext = null) { - await AfterAnyTearDowns.Invoke(this, new TestHookIMethodEventArgs(context, method, exceptionContext)); + AfterAnyTearDowns.Invoke(this, new TestHookIMethodEventArgs(context, method, exceptionContext)); } }