From abe1678c6b2ebf0bd247f5116904a8d8ccf61cb4 Mon Sep 17 00:00:00 2001 From: Automation51D Date: Tue, 12 Sep 2023 11:02:39 +0100 Subject: [PATCH 1/2] TEST: Instead of using time taken to determine if a test is cancelled, set and check variables instead. This is a more reliable and accurate method of testing as it is not succeptable to the speed of a processor. --- .../Loaders/TrackingLoaderBase.cs | 17 ++- .../LoadingDictionaryTests.cs | 110 +++++++++++++++--- 2 files changed, 111 insertions(+), 16 deletions(-) diff --git a/FiftyOne.Caching.Tests/Loaders/TrackingLoaderBase.cs b/FiftyOne.Caching.Tests/Loaders/TrackingLoaderBase.cs index e7587e2..3b3b629 100644 --- a/FiftyOne.Caching.Tests/Loaders/TrackingLoaderBase.cs +++ b/FiftyOne.Caching.Tests/Loaders/TrackingLoaderBase.cs @@ -36,10 +36,18 @@ internal abstract class TrackingLoaderBase : IValueTaskLoader _calls; public int Cancels => _cancels; + public int CompleteWaits => _completeWaits; + + public int TaskStarts => _taskStarts; + public TrackingLoaderBase() : this(0) { @@ -56,14 +64,19 @@ public Task Load(TKey key, CancellationToken token) Interlocked.Increment(ref _calls); return Task.Run(() => { + Interlocked.Increment(ref _taskStarts); if (_delayMillis > 0) { var start = DateTime.Now; - while (DateTime.Now < start.AddMilliseconds(_delayMillis) && + while (DateTime.Now <= start.AddMilliseconds(_delayMillis) && token.IsCancellationRequested == false) { Thread.Sleep(1); } + if (DateTime.Now >= start.AddMilliseconds(_delayMillis)) + { + Interlocked.Increment(ref _completeWaits); + } if (token.IsCancellationRequested) { Interlocked.Increment(ref _cancels); @@ -71,7 +84,7 @@ public Task Load(TKey key, CancellationToken token) } } return GetValue(key); - }); + }, token); } public TValue Load(TKey key) diff --git a/FiftyOne.Caching.Tests/LoadingDictionaryTests.cs b/FiftyOne.Caching.Tests/LoadingDictionaryTests.cs index 9f14ab7..3611ab3 100644 --- a/FiftyOne.Caching.Tests/LoadingDictionaryTests.cs +++ b/FiftyOne.Caching.Tests/LoadingDictionaryTests.cs @@ -354,8 +354,8 @@ public void LoadingDictionary_TryGetFails() Assert.IsFalse(success); Assert.AreEqual(0, dict.Keys.Count()); - } - + } + /// /// Test that calling the cancellation token results in the get method /// returning immediately, and not waiting for the Task to complete. @@ -367,6 +367,49 @@ public void LoadingDictionary_GetCancelled() { // Arrange + var millis = 1000; + var value = "teststring"; + var loader = new ReturnKeyLoader(millis); + var dict = new LoadingDictionary(_logger.Object, loader); + Exception exception = null; + + // Act + + var getter = Task.Run(() => dict[value, _token.Token]); + _token.Cancel(); + try + { + getter.Wait(millis * 2); + Assert.Fail( + "The prior cancel of the token should prevent getting here"); + } + catch (AggregateException ex) + { + Assert.IsTrue(ex.InnerExceptions.Count == 1); + Assert.IsTrue(typeof(TaskCanceledException) == ex.InnerException.GetType()); + exception = ex.InnerException; + } + + // Assert + + Assert.AreEqual(1, loader.TaskStarts); + Assert.AreEqual(0, loader.CompleteWaits); + Assert.AreEqual(1, loader.Cancels); + Assert.IsNotNull(exception); + } + + /// + /// Test that calling the get method with a cancellation token that + /// is already cancelled results in it returning immediately, and + /// does not even start the task. + /// Also check that the cancellation was passed to the loader properly, + /// and that the correct exception is thrown. + /// + [TestMethod] + public void LoadingDictionary_GetAlreadyCancelled() + { + // Arrange + var millis = 1000; var value = "teststring"; var loader = new ReturnKeyLoader(millis); @@ -375,7 +418,6 @@ public void LoadingDictionary_GetCancelled() // Act - var start = DateTime.Now; _token.Cancel(); try { @@ -384,25 +426,67 @@ public void LoadingDictionary_GetCancelled() catch (OperationCanceledException e) { exception = e; + } + + // Assert + + Assert.AreEqual(0, loader.TaskStarts); + Assert.AreEqual(0, loader.CompleteWaits); + Assert.AreEqual(0, loader.Cancels); + Assert.IsNotNull(exception); + } + + /// + /// Test that calling the cancellation token results in the TryGet method + /// returning immediately, and not waiting for the Task to complete. + /// Also check that the cancellation was passed to the loader properly, + /// and that the correct exception is thrown. + /// + [TestMethod] + public void LoadingDictionary_TryGetCancelled() + { + // Arrange + + var millis = 1000; + var value = "teststring"; + var loader = new ReturnKeyLoader(millis); + var dict = new LoadingDictionary(_logger.Object, loader); + Exception exception = null; + + // Act + + var getter = Task.Run(() => dict.TryGet(value, _token.Token, out _)); + _token.Cancel(); + try + { + getter.Wait(millis * 2); + Assert.Fail( + "The prior cancel of the token should prevent getting here"); + } + catch (AggregateException ex) + { + Assert.IsTrue(ex.InnerExceptions.Count == 1); + Assert.IsTrue(typeof(TaskCanceledException) == ex.InnerException.GetType()); + exception = ex.InnerException; } - var end = DateTime.Now; - Thread.Sleep(100); // Assert - Assert.IsTrue((end - start).TotalMilliseconds < millis); + Assert.AreEqual(1, loader.TaskStarts); + Assert.AreEqual(0, loader.CompleteWaits); Assert.AreEqual(1, loader.Cancels); Assert.IsNotNull(exception); } /// - /// Test that calling the cancellation token results in the TryGet method - /// returning immediately, and not waiting for the Task to complete. + /// Test that calling the TryGet method with a cancellation token that + /// is already cancelled results in it returning immediately, and + /// does not even start the task. /// Also check that the cancellation was passed to the loader properly, /// and that the correct exception is thrown. /// [TestMethod] - public void LoadingDictionary_TryGetCancelled() + public void LoadingDictionary_TryGetAlreadyCancelled() { // Arrange @@ -414,7 +498,6 @@ public void LoadingDictionary_TryGetCancelled() // Act - var start = DateTime.Now; _token.Cancel(); try { @@ -424,13 +507,12 @@ public void LoadingDictionary_TryGetCancelled() { exception = e; } - var end = DateTime.Now; - Thread.Sleep(100); // Assert - Assert.IsTrue((end - start).TotalMilliseconds < millis); - Assert.AreEqual(1, loader.Cancels); + Assert.AreEqual(0, loader.TaskStarts); + Assert.AreEqual(0, loader.CompleteWaits); + Assert.AreEqual(0, loader.Cancels); Assert.IsNotNull(exception); } From d31baf7a9e6bb030671ff596c67b9c129222e4ff Mon Sep 17 00:00:00 2001 From: Automation51D Date: Tue, 12 Sep 2023 11:38:17 +0100 Subject: [PATCH 2/2] BUG: Only pass the token to Task.Run for tests which are checking for that. The risk is that the token is cancelled before the task has been started, then we cannot see what's happened in the task. Toggling this gives us control of what were testing without relying on time. --- .../Loaders/ReturnKeyLoader.cs | 6 ++- .../Loaders/TrackingLoaderBase.cs | 15 +++---- .../LoadingDictionaryTests.cs | 39 +++++++++---------- 3 files changed, 31 insertions(+), 29 deletions(-) diff --git a/FiftyOne.Caching.Tests/Loaders/ReturnKeyLoader.cs b/FiftyOne.Caching.Tests/Loaders/ReturnKeyLoader.cs index 0f716b4..ffa63c6 100644 --- a/FiftyOne.Caching.Tests/Loaders/ReturnKeyLoader.cs +++ b/FiftyOne.Caching.Tests/Loaders/ReturnKeyLoader.cs @@ -42,9 +42,13 @@ internal class ReturnKeyLoader : TrackingLoaderBase { public ReturnKeyLoader() { + } + + public ReturnKeyLoader(int delayMillis) : base(delayMillis) + { } - public ReturnKeyLoader(int delayMillis) : base(delayMillis) + public ReturnKeyLoader(int delayMillis, bool runWithToken) : base(delayMillis, runWithToken) { } diff --git a/FiftyOne.Caching.Tests/Loaders/TrackingLoaderBase.cs b/FiftyOne.Caching.Tests/Loaders/TrackingLoaderBase.cs index 3b3b629..06cfb83 100644 --- a/FiftyOne.Caching.Tests/Loaders/TrackingLoaderBase.cs +++ b/FiftyOne.Caching.Tests/Loaders/TrackingLoaderBase.cs @@ -38,7 +38,7 @@ internal abstract class TrackingLoaderBase : IValueTaskLoader _calls; @@ -46,25 +46,26 @@ internal abstract class TrackingLoaderBase : IValueTaskLoader _completeWaits; - public int TaskStarts => _taskStarts; - public TrackingLoaderBase() : this(0) { } - public TrackingLoaderBase(int delayMillis) + public TrackingLoaderBase(int delayMillis) : this(delayMillis, false) { - _delayMillis = delayMillis; } + public TrackingLoaderBase(int delayMillis, bool runWithToken) + { + _delayMillis = delayMillis; + _runWithToken = runWithToken; + } public Task Load(TKey key, CancellationToken token) { Interlocked.Increment(ref _calls); return Task.Run(() => { - Interlocked.Increment(ref _taskStarts); if (_delayMillis > 0) { var start = DateTime.Now; @@ -84,7 +85,7 @@ public Task Load(TKey key, CancellationToken token) } } return GetValue(key); - }, token); + }, _runWithToken ? token : new CancellationToken()); } public TValue Load(TKey key) diff --git a/FiftyOne.Caching.Tests/LoadingDictionaryTests.cs b/FiftyOne.Caching.Tests/LoadingDictionaryTests.cs index 3611ab3..09db576 100644 --- a/FiftyOne.Caching.Tests/LoadingDictionaryTests.cs +++ b/FiftyOne.Caching.Tests/LoadingDictionaryTests.cs @@ -367,7 +367,7 @@ public void LoadingDictionary_GetCancelled() { // Arrange - var millis = 1000; + var millis = 5000; var value = "teststring"; var loader = new ReturnKeyLoader(millis); var dict = new LoadingDictionary(_logger.Object, loader); @@ -392,7 +392,6 @@ public void LoadingDictionary_GetCancelled() // Assert - Assert.AreEqual(1, loader.TaskStarts); Assert.AreEqual(0, loader.CompleteWaits); Assert.AreEqual(1, loader.Cancels); Assert.IsNotNull(exception); @@ -410,9 +409,9 @@ public void LoadingDictionary_GetAlreadyCancelled() { // Arrange - var millis = 1000; + var millis = 5000; var value = "teststring"; - var loader = new ReturnKeyLoader(millis); + var loader = new ReturnKeyLoader(millis, true); var dict = new LoadingDictionary(_logger.Object, loader); OperationCanceledException exception = null; @@ -430,7 +429,6 @@ public void LoadingDictionary_GetAlreadyCancelled() // Assert - Assert.AreEqual(0, loader.TaskStarts); Assert.AreEqual(0, loader.CompleteWaits); Assert.AreEqual(0, loader.Cancels); Assert.IsNotNull(exception); @@ -447,7 +445,7 @@ public void LoadingDictionary_TryGetCancelled() { // Arrange - var millis = 1000; + var millis = 5000; var value = "teststring"; var loader = new ReturnKeyLoader(millis); var dict = new LoadingDictionary(_logger.Object, loader); @@ -472,7 +470,6 @@ public void LoadingDictionary_TryGetCancelled() // Assert - Assert.AreEqual(1, loader.TaskStarts); Assert.AreEqual(0, loader.CompleteWaits); Assert.AreEqual(1, loader.Cancels); Assert.IsNotNull(exception); @@ -490,9 +487,9 @@ public void LoadingDictionary_TryGetAlreadyCancelled() { // Arrange - var millis = 1000; + var millis = 5000; var value = "teststring"; - var loader = new ReturnKeyLoader(millis); + var loader = new ReturnKeyLoader(millis, true); var dict = new LoadingDictionary(_logger.Object, loader); OperationCanceledException exception = null; @@ -510,7 +507,6 @@ public void LoadingDictionary_TryGetAlreadyCancelled() // Assert - Assert.AreEqual(0, loader.TaskStarts); Assert.AreEqual(0, loader.CompleteWaits); Assert.AreEqual(0, loader.Cancels); Assert.IsNotNull(exception); @@ -661,25 +657,26 @@ public void LoadingDictionary_GetCanceledIsNotReused() // Arrange var value = "testvalue"; - var millis = 1000; + var millis = 5000; var loader = new ReturnKeyLoader(millis); var dict = new LoadingDictionary(_logger.Object, loader); - OperationCanceledException exception = null; + Exception exception = null; var count = 2; // Act for (int i = 0; i < count; i++) { - _token.Cancel(); + var getter = Task.Run(() => _ = dict[value, _token.Token]); + _token.Cancel(); try { - _ = dict[value, _token.Token]; + _ = getter.Result; } - catch (OperationCanceledException e) + catch (AggregateException e) { - exception = e; + exception = e.InnerException; } Assert.IsNotNull(exception); exception = null; @@ -736,7 +733,7 @@ public void LoadingDictionary_TryGetCanceledIsRemoved() // Arrange var value = "testvalue"; - var millis = 1000; + var millis = 5000; var loader = new ReturnKeyLoader(millis); var dict = new LoadingDictionary(_logger.Object, loader); var count = 2; @@ -744,16 +741,16 @@ public void LoadingDictionary_TryGetCanceledIsRemoved() // Act for (int i = 0; i < count; i++) - { + { + var getter = Task.Run(() => dict.TryGet(value, _token.Token, out _)); _token.Cancel(); - Assert.ThrowsException(() => + Assert.ThrowsException(() => { - _ = dict.TryGet(value, _token.Token, out _); + _ = getter.Result; }); _token = new CancellationTokenSource(); - Thread.Sleep(100); } // Assert