From 1bc41e09616585fb21ad5887586b1b00d6b834f7 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Tue, 9 Apr 2019 14:03:12 -0700 Subject: [PATCH] Add Repeat attribute (#1375) --- .../src/LoggedTest/LoggedTestBase.cs | 2 +- .../src/{RetryContext.cs => RepeatContext.cs} | 6 +- .../src/Xunit/LoggedTestInvoker.cs | 20 +--- .../src/Xunit/LoggedTestRunner.cs | 100 ++++++++--------- .../src/Xunit/RepeatAttribute.cs | 28 +++++ .../src/Xunit/RetryTestAttribute.cs | 68 ------------ .../test/LoggedTestXunitRepeatTests.cs | 43 ++++++++ .../test/LoggedTestXunitRetryTests.cs | 101 ------------------ .../test/LoggedTestXunitTests.cs | 2 +- .../test/Properties/AssemblyInfo.cs | 1 + 10 files changed, 121 insertions(+), 250 deletions(-) rename src/Logging/Logging.Testing/src/{RetryContext.cs => RepeatContext.cs} (70%) create mode 100644 src/Logging/Logging.Testing/src/Xunit/RepeatAttribute.cs delete mode 100644 src/Logging/Logging.Testing/src/Xunit/RetryTestAttribute.cs create mode 100644 src/Logging/Logging.Testing/test/LoggedTestXunitRepeatTests.cs delete mode 100644 src/Logging/Logging.Testing/test/LoggedTestXunitRetryTests.cs diff --git a/src/Logging/Logging.Testing/src/LoggedTest/LoggedTestBase.cs b/src/Logging/Logging.Testing/src/LoggedTest/LoggedTestBase.cs index 492de61cb6..94cdf82257 100644 --- a/src/Logging/Logging.Testing/src/LoggedTest/LoggedTestBase.cs +++ b/src/Logging/Logging.Testing/src/LoggedTest/LoggedTestBase.cs @@ -26,7 +26,7 @@ namespace Microsoft.Extensions.Logging.Testing // Internal for testing internal string ResolvedTestClassName { get; set; } - internal RetryContext RetryContext { get; set; } + internal RepeatContext RepeatContext { get; set; } public string ResolvedLogOutputDirectory { get; set; } diff --git a/src/Logging/Logging.Testing/src/RetryContext.cs b/src/Logging/Logging.Testing/src/RepeatContext.cs similarity index 70% rename from src/Logging/Logging.Testing/src/RetryContext.cs rename to src/Logging/Logging.Testing/src/RepeatContext.cs index 529de656b5..decc7a173c 100644 --- a/src/Logging/Logging.Testing/src/RetryContext.cs +++ b/src/Logging/Logging.Testing/src/RepeatContext.cs @@ -3,14 +3,10 @@ namespace Microsoft.Extensions.Logging.Testing { - public class RetryContext + public class RepeatContext { internal int Limit { get; set; } - internal object TestClassInstance { get; set; } - - internal string Reason { get; set; } - internal int CurrentIteration { get; set; } } } diff --git a/src/Logging/Logging.Testing/src/Xunit/LoggedTestInvoker.cs b/src/Logging/Logging.Testing/src/Xunit/LoggedTestInvoker.cs index 788c38b306..0e6638cb57 100644 --- a/src/Logging/Logging.Testing/src/Xunit/LoggedTestInvoker.cs +++ b/src/Logging/Logging.Testing/src/Xunit/LoggedTestInvoker.cs @@ -16,7 +16,7 @@ namespace Microsoft.Extensions.Logging.Testing public class LoggedTestInvoker : XunitTestInvoker { private readonly ITestOutputHelper _output; - private readonly RetryContext _retryContext; + private readonly RepeatContext _repeatContext; private readonly bool _collectDumpOnFailure; public LoggedTestInvoker( @@ -30,12 +30,12 @@ namespace Microsoft.Extensions.Logging.Testing ExceptionAggregator aggregator, CancellationTokenSource cancellationTokenSource, ITestOutputHelper output, - RetryContext retryContext, + RepeatContext repeatContext, bool collectDumpOnFailure) : base(test, messageBus, testClass, constructorArguments, testMethod, testMethodArguments, beforeAfterAttributes, aggregator, cancellationTokenSource) { _output = output; - _retryContext = retryContext; + _repeatContext = repeatContext; _collectDumpOnFailure = collectDumpOnFailure; } @@ -51,19 +51,7 @@ namespace Microsoft.Extensions.Logging.Testing if (testClass is LoggedTestBase loggedTestBase) { // Used for testing - loggedTestBase.RetryContext = _retryContext; - - if (_retryContext != null) - { - // Log retry attempt as warning - if (_retryContext.CurrentIteration > 0) - { - loggedTestBase.Logger.LogWarning($"{TestMethod.Name} failed and retry conditions are met, re-executing. The reason for failure is {_retryContext.Reason}."); - } - - // Save the test class instance for non-static predicates - _retryContext.TestClassInstance = testClass; - } + loggedTestBase.RepeatContext = _repeatContext; } return testClass; diff --git a/src/Logging/Logging.Testing/src/Xunit/LoggedTestRunner.cs b/src/Logging/Logging.Testing/src/Xunit/LoggedTestRunner.cs index 11537eb679..8069801fbe 100644 --- a/src/Logging/Logging.Testing/src/Xunit/LoggedTestRunner.cs +++ b/src/Logging/Logging.Testing/src/Xunit/LoggedTestRunner.cs @@ -49,82 +49,66 @@ namespace Microsoft.Extensions.Logging.Testing private async Task InvokeTestMethodAsync(ExceptionAggregator aggregator, ITestOutputHelper output) { - var retryAttribute = GetRetryAttribute(TestMethod); var collectDump = TestMethod.GetCustomAttribute() != null; - - if (!typeof(LoggedTestBase).IsAssignableFrom(TestClass) || retryAttribute == null) + var repeatAttribute = GetRepeatAttribute(TestMethod); + + if (!typeof(LoggedTestBase).IsAssignableFrom(TestClass) || repeatAttribute == null) { return await new LoggedTestInvoker(Test, MessageBus, TestClass, ConstructorArguments, TestMethod, TestMethodArguments, BeforeAfterAttributes, aggregator, CancellationTokenSource, output, null, collectDump).RunAsync(); } - var retryPredicateMethodName = retryAttribute.RetryPredicateName; - var retryPredicateMethod = TestClass.GetMethod(retryPredicateMethodName, - BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.Static, - null, - new Type[] { typeof(Exception) }, - null) - ?? throw new InvalidOperationException($"No valid static retry predicate method {retryPredicateMethodName} was found on the type {TestClass.FullName}."); - - if (retryPredicateMethod.ReturnType != typeof(bool)) - { - throw new InvalidOperationException($"Retry predicate method {retryPredicateMethodName} on {TestClass.FullName} does not return bool."); - } - - var retryContext = new RetryContext() - { - Limit = retryAttribute.RetryLimit, - Reason = retryAttribute.RetryReason, - }; - - var retryAggregator = new ExceptionAggregator(); - var loggedTestInvoker = new LoggedTestInvoker(Test, MessageBus, TestClass, ConstructorArguments, TestMethod, TestMethodArguments, BeforeAfterAttributes, retryAggregator, CancellationTokenSource, output, retryContext, collectDump); - var totalTime = 0.0M; - - do - { - retryAggregator.Clear(); - totalTime += await loggedTestInvoker.RunAsync(); - retryContext.CurrentIteration++; - } - while (retryAggregator.HasExceptions - && retryContext.CurrentIteration < retryContext.Limit - && (retryPredicateMethod.IsStatic - ? (bool)retryPredicateMethod.Invoke(null, new object[] { retryAggregator.ToException() }) - : (bool)retryPredicateMethod.Invoke(retryContext.TestClassInstance, new object[] { retryAggregator.ToException() })) - ); - - aggregator.Aggregate(retryAggregator); - return totalTime; + return await RunRepeatTestInvoker(aggregator, output, collectDump, repeatAttribute); } - private RetryTestAttribute GetRetryAttribute(MethodInfo methodInfo) + private async Task RunRepeatTestInvoker(ExceptionAggregator aggregator, ITestOutputHelper output, bool collectDump, RepeatAttribute repeatAttribute) { - var os = RuntimeInformation.IsOSPlatform(OSPlatform.OSX) ? OperatingSystems.MacOSX - : RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? OperatingSystems.Windows - : OperatingSystems.Linux; + var repeatContext = new RepeatContext + { + Limit = repeatAttribute.RunCount + }; - var attributeCandidate = methodInfo.GetCustomAttribute(); + var timeTaken = 0.0M; + var testLogger = new LoggedTestInvoker( + Test, + MessageBus, + TestClass, + ConstructorArguments, + TestMethod, + TestMethodArguments, + BeforeAfterAttributes, + aggregator, + CancellationTokenSource, + output, + repeatContext, + collectDump); - if (attributeCandidate != null && (attributeCandidate.OperatingSystems & os) != 0) + for (repeatContext.CurrentIteration = 0; repeatContext.CurrentIteration < repeatContext.Limit; repeatContext.CurrentIteration++) + { + timeTaken = await testLogger.RunAsync(); + if (aggregator.HasExceptions) + { + return timeTaken; + } + } + + return timeTaken; + } + + private RepeatAttribute GetRepeatAttribute(MethodInfo methodInfo) + { + var attributeCandidate = methodInfo.GetCustomAttribute(); + if (attributeCandidate != null) { return attributeCandidate; } - attributeCandidate = methodInfo.DeclaringType.GetCustomAttribute(); - - if (attributeCandidate != null && (attributeCandidate.OperatingSystems & os) != 0) + attributeCandidate = methodInfo.DeclaringType.GetCustomAttribute(); + if (attributeCandidate != null) { return attributeCandidate; } - attributeCandidate = methodInfo.DeclaringType.Assembly.GetCustomAttribute(); - - if (attributeCandidate != null && (attributeCandidate.OperatingSystems & os) != 0) - { - return attributeCandidate; - } - - return null; + return methodInfo.DeclaringType.Assembly.GetCustomAttribute(); } } } diff --git a/src/Logging/Logging.Testing/src/Xunit/RepeatAttribute.cs b/src/Logging/Logging.Testing/src/Xunit/RepeatAttribute.cs new file mode 100644 index 0000000000..ec86c0b601 --- /dev/null +++ b/src/Logging/Logging.Testing/src/Xunit/RepeatAttribute.cs @@ -0,0 +1,28 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.ComponentModel; + +namespace Microsoft.Extensions.Logging.Testing +{ + /// + /// Runs a test multiple times to stress flaky tests that are believed to be fixed. + /// This can be used on an assembly, class, or method name. + /// Requires using to run. + /// + [EditorBrowsable(EditorBrowsableState.Never)] + [AttributeUsage(AttributeTargets.Method | AttributeTargets.Class | AttributeTargets.Assembly, AllowMultiple = false)] + public class RepeatAttribute : Attribute + { + public RepeatAttribute(int runCount = 10) + { + RunCount = runCount; + } + + /// + /// The number of times to run a test. + /// + public int RunCount { get; } + } +} diff --git a/src/Logging/Logging.Testing/src/Xunit/RetryTestAttribute.cs b/src/Logging/Logging.Testing/src/Xunit/RetryTestAttribute.cs deleted file mode 100644 index e85f2c517e..0000000000 --- a/src/Logging/Logging.Testing/src/Xunit/RetryTestAttribute.cs +++ /dev/null @@ -1,68 +0,0 @@ -// Copyright (c) .NET Foundation. All rights reserved. -// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. - -using System; -using System.ComponentModel; -using Microsoft.AspNetCore.Testing.xunit; - -namespace Microsoft.Extensions.Logging.Testing -{ - /// - /// WARNING: This attribute should only be used on well understood flaky test failures caused by external issues and should be removed once the underlying issues have been resolved. - /// This is not intended to be a long term solution to ensure passing of flaky tests but instead a method to improve test reliability without reducing coverage. - /// Issues should be filed to remove these attributes from affected tests as soon as the underlying issue is fixed. - /// - [EditorBrowsable(EditorBrowsableState.Never)] - [AttributeUsage(AttributeTargets.Method | AttributeTargets.Class, AllowMultiple = false)] - public class RetryTestAttribute : Attribute - { - /// - /// WARNING: This attribute should only be used on well understood flaky test failures caused by external issues and should be removed once the underlying issues have been resolved. - /// This is not intended to be a long term solution to ensure passing of flaky tests but instead a method to improve test reliability without reducing coverage. - /// Issues should be filed to remove these attributes from affected tests as soon as the underlying issue is fixed. - /// - /// The predicate of the format Func<Exception,bool> that is used to determine if the test should be retried - /// The reason for retrying the test - /// The number of retries to attempt before failing the test, for most purposes this this should be kept at 2 to avoid excessive retries. - public RetryTestAttribute(string retryPredicateName, string retryReason, int retryLimit = 2) - : this(retryPredicateName, retryReason, OperatingSystems.Linux | OperatingSystems.MacOSX | OperatingSystems.Windows, retryLimit) { } - - /// - /// WARNING: This attribute should only be used on well understood flaky test failures caused by external issuesand should be removed once the underlying issues have been resolved. - /// This is not intended to be a long term solution to ensure passing of flaky tests but instead a method to improve test reliability without reducing coverage. - /// Issues should be filed to remove these attributes from affected tests as soon as the underlying issue is fixed. - /// - /// The os(es) this retry should be attempted on. - /// The predicate of the format Func<Exception,bool> that is used to determine if the test should be retried - /// The reason for retrying the test - /// The number of retries to attempt before failing the test, for most purposes this this should be kept at 2 to avoid excessive retries. - public RetryTestAttribute(string retryPredicateName, string retryReason, OperatingSystems operatingSystems, int retryLimit = 2) - { - if (string.IsNullOrEmpty(retryPredicateName)) - { - throw new ArgumentNullException(nameof(RetryPredicateName), "A valid non-empty predicate method name must be provided."); - } - if (string.IsNullOrEmpty(retryReason)) - { - throw new ArgumentNullException(nameof(retryReason), "A valid non-empty reason for retrying the test must be provided."); - } - if (retryLimit < 1) - { - throw new ArgumentOutOfRangeException(nameof(retryLimit), retryLimit, "Retry count must be positive."); - } - - OperatingSystems = operatingSystems; - RetryPredicateName = retryPredicateName; - RetryReason = retryReason; - RetryLimit = retryLimit; - } - - public string RetryPredicateName { get; } - - public string RetryReason { get; } - - public int RetryLimit { get; } - - public OperatingSystems OperatingSystems { get; } - } -} diff --git a/src/Logging/Logging.Testing/test/LoggedTestXunitRepeatTests.cs b/src/Logging/Logging.Testing/test/LoggedTestXunitRepeatTests.cs new file mode 100644 index 0000000000..dbd1d7260a --- /dev/null +++ b/src/Logging/Logging.Testing/test/LoggedTestXunitRepeatTests.cs @@ -0,0 +1,43 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Xunit; + +namespace Microsoft.Extensions.Logging.Testing.Tests +{ + [Repeat] + public class LoggedTestXunitRepeatTests : LoggedTest + { + public static int _runCount = 0; + + [Fact] + [Repeat(5)] + public void RepeatLimitIsSetCorrectly() + { + Assert.Equal(5, RepeatContext.Limit); + } + + [Fact] + [Repeat(5)] + public void RepeatRunsTestSpecifiedNumberOfTimes() + { + Assert.Equal(RepeatContext.CurrentIteration, _runCount); + _runCount++; + } + + [Fact] + public void RepeatCanBeSetOnClass() + { + Assert.Equal(10, RepeatContext.Limit); + } + } + + public class LoggedTestXunitRepeatAssemblyTests : LoggedTest + { + [Fact] + public void RepeatCanBeSetOnAssembly() + { + Assert.Equal(1, RepeatContext.Limit); + } + } +} diff --git a/src/Logging/Logging.Testing/test/LoggedTestXunitRetryTests.cs b/src/Logging/Logging.Testing/test/LoggedTestXunitRetryTests.cs deleted file mode 100644 index 1e49f45046..0000000000 --- a/src/Logging/Logging.Testing/test/LoggedTestXunitRetryTests.cs +++ /dev/null @@ -1,101 +0,0 @@ -// Copyright (c) .NET Foundation. All rights reserved. -// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. - -using System; -using System.Linq; -using Microsoft.AspNetCore.Testing.xunit; -using Xunit; - -namespace Microsoft.Extensions.Logging.Testing.Tests -{ - [RetryTest(nameof(RetryAllPredicate), "sample reason")] - public class LoggedTestXunitRetryTests : LoggedTest - { - [Fact] - public void CompletesWithoutRetryOnSuccess() - { - Assert.Equal(2, RetryContext.Limit); - - // This assert would fail on the second run - Assert.Equal(0, RetryContext.CurrentIteration); - } - - [Fact] - public void RetriesUntilSuccess() - { - // This assert will fail the first time but pass on the second - Assert.Equal(1, RetryContext.CurrentIteration); - - // This assert will ensure a message is logged for retried tests. - Assert.Equal(1, TestSink.Writes.Count); - var loggedMessage = TestSink.Writes.ToArray()[0]; - Assert.Equal(LogLevel.Warning, loggedMessage.LogLevel); - Assert.Equal($"{nameof(RetriesUntilSuccess)} failed and retry conditions are met, re-executing. The reason for failure is sample reason.", loggedMessage.Message); - } - - [ConditionalFact] - [OSSkipCondition(OperatingSystems.Windows)] - [RetryTest(nameof(RetryAllPredicate), "sample reason", OperatingSystems.Windows, 3)] - public void RetryCountNotOverridenWhenOSDoesNotMatch() - { - Assert.Equal(2, RetryContext.Limit); - } - - [ConditionalFact] - [OSSkipCondition(OperatingSystems.Linux | OperatingSystems.MacOSX)] - [RetryTest(nameof(RetryAllPredicate), "sample reason", OperatingSystems.Windows, 3)] - public void RetryCountOverridenWhenOSMatches() - { - Assert.Equal(3, RetryContext.Limit); - } - - [Fact] - [RetryTest(nameof(RetryInvalidOperationExceptionPredicate), "sample reason")] - public void RetryIfPredicateIsTrue() - { - if (RetryContext.CurrentIteration == 0) - { - Logger.LogWarning("Throw on first iteration"); - throw new Exception(); - } - - // This assert will ensure a message is logged for retried tests. - Assert.Equal(1, TestSink.Writes.Count); - var loggedMessage = TestSink.Writes.ToArray()[0]; - Assert.Equal(LogLevel.Warning, loggedMessage.LogLevel); - Assert.Equal($"{nameof(RetryIfPredicateIsTrue)} failed and retry conditions are met, re-executing. The reason for failure is sample reason.", loggedMessage.Message); - } - - // Static predicates are valid - private static bool RetryAllPredicate(Exception e) - => true; - - // Instance predicates are valid - private bool RetryInvalidOperationExceptionPredicate(Exception e) - => TestSink.Writes.Any(m => m.Message.Contains("Throw on first iteration")); - } - - [RetryTest(nameof(RetryAllPredicate), "sample reason")] - public class LoggedTestXunitRetryConstructorTest : LoggedTest - { - private static int _constructorInvocationCount; - - public LoggedTestXunitRetryConstructorTest() - { - _constructorInvocationCount++; - } - - [Fact] - public void RetriesUntilSuccess() - { - // The constructor is invoked before the test method but the current iteration is updated after - Assert.Equal(_constructorInvocationCount, RetryContext.CurrentIteration + 1); - - // This assert will fail the first time but pass on the second - Assert.Equal(1, RetryContext.CurrentIteration); - } - - private static bool RetryAllPredicate(Exception e) - => true; - } -} diff --git a/src/Logging/Logging.Testing/test/LoggedTestXunitTests.cs b/src/Logging/Logging.Testing/test/LoggedTestXunitTests.cs index 507453a242..520ffaaa9e 100644 --- a/src/Logging/Logging.Testing/test/LoggedTestXunitTests.cs +++ b/src/Logging/Logging.Testing/test/LoggedTestXunitTests.cs @@ -1,4 +1,4 @@ -// Copyright (c) .NET Foundation. All rights reserved. +// Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.Linq; diff --git a/src/Logging/Logging.Testing/test/Properties/AssemblyInfo.cs b/src/Logging/Logging.Testing/test/Properties/AssemblyInfo.cs index 82616e2737..b104c11dfc 100644 --- a/src/Logging/Logging.Testing/test/Properties/AssemblyInfo.cs +++ b/src/Logging/Logging.Testing/test/Properties/AssemblyInfo.cs @@ -2,3 +2,4 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Testing; [assembly: LogLevel(LogLevel.Trace)] +[assembly: Repeat(1)]