Merge pull request ppy#6253 from smoogipoo/fix-flaky-tests

Fix flaky test attribute not working

osu.Framework.Tests/FlakyTestAttribute.cs

@@ -1,7 +1,6 @@
 // Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
-using System;
 using NUnit.Framework;
 
 namespace osu.Framework.Tests
@@ -18,7 +17,7 @@ public FlakyTestAttribute()
         }
 
         public FlakyTestAttribute(int tryCount)
-            : base(Environment.GetEnvironmentVariable("OSU_TESTS_FAIL_FLAKY") == "1" ? 1 : tryCount)
+            : base(FrameworkEnvironment.FailFlakyTests ? 1 : tryCount)
         {
         }
     }

osu.Framework.Tests/Visual/Testing/TestSceneNestedGame.cs

@@ -81,10 +81,8 @@ public void TearDownSteps()
             AddStep("mark host running", () => hostWasRunningAfterNestedExit = true);
         }
 
-        public override void RunTestsFromNUnit()
+        internal override void RunAfterTest()
         {
-            base.RunTestsFromNUnit();
-
             Assert.IsTrue(hostWasRunningAfterNestedExit);
         }
 

osu.Framework.Tests/Visual/Testing/TestSceneTestRetry.cs

@@ -0,0 +1,49 @@
+// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+using NUnit.Framework;
+using NUnit.Framework.Internal;
+using osu.Framework.Testing;
+
+namespace osu.Framework.Tests.Visual.Testing
+{
+    [HeadlessTest]
+    public partial class TestSceneTestRetry : FrameworkTestScene
+    {
+        private int runCount;
+        private string? currentTest;
+
+        [OneTimeSetUp]
+        public void OneTimeSetup()
+        {
+            if (FrameworkEnvironment.FailFlakyTests)
+                Assert.Ignore("Can't run while failing flaky tests.");
+        }
+
+        [SetUp]
+        public void Setup() => Schedule(() =>
+        {
+            if (currentTest == TestExecutionContext.CurrentContext.CurrentTest.Name)
+                return;
+
+            runCount = 0;
+            currentTest = TestExecutionContext.CurrentContext.CurrentTest.Name;
+        });
+
+        [Test]
+        [FlakyTest(10)]
+        public void FlakyTestWithAssert()
+        {
+            AddStep("increment", () => runCount++);
+            AddAssert("assert if not ran 5 times", () => runCount, () => Is.EqualTo(5));
+        }
+
+        [Test]
+        [FlakyTest(3)]
+        public void FlakyTestWithUntilStep()
+        {
+            AddStep("increment", () => runCount++);
+            AddUntilStep("assert if not ran 2 times", () => runCount, () => Is.EqualTo(2));
+        }
+    }
+}

osu.Framework/FrameworkEnvironment.cs

@@ -11,6 +11,7 @@ public static class FrameworkEnvironment
         public static ExecutionMode? StartupExecutionMode { get; }
         public static bool NoTestTimeout { get; }
         public static bool ForceTestGC { get; }
+        public static bool FailFlakyTests { get; }
         public static bool FrameStatisticsViaTouch { get; }
         public static GraphicsSurfaceType? PreferredGraphicsSurface { get; }
         public static string? PreferredGraphicsRenderer { get; }
@@ -22,8 +23,11 @@ public static class FrameworkEnvironment
         static FrameworkEnvironment()
         {
             StartupExecutionMode = Enum.TryParse<ExecutionMode>(Environment.GetEnvironmentVariable("OSU_EXECUTION_MODE"), true, out var mode) ? mode : null;
+
             NoTestTimeout = parseBool(Environment.GetEnvironmentVariable("OSU_TESTS_NO_TIMEOUT")) ?? false;
             ForceTestGC = parseBool(Environment.GetEnvironmentVariable("OSU_TESTS_FORCED_GC")) ?? false;
+            FailFlakyTests = Environment.GetEnvironmentVariable("OSU_TESTS_FAIL_FLAKY") == "1";
+
             FrameStatisticsViaTouch = parseBool(Environment.GetEnvironmentVariable("OSU_FRAME_STATISTICS_VIA_TOUCH")) ?? true;
             PreferredGraphicsSurface = Enum.TryParse<GraphicsSurfaceType>(Environment.GetEnvironmentVariable("OSU_GRAPHICS_SURFACE"), true, out var surface) ? surface : null;
             PreferredGraphicsRenderer = Environment.GetEnvironmentVariable("OSU_GRAPHICS_RENDERER")?.ToLowerInvariant();

osu.Framework/Testing/Drawables/Steps/AssertButton.cs

@@ -6,6 +6,7 @@
 using System;
 using System.Diagnostics;
 using System.Text;
+using NUnit.Framework;
 using osuTK.Graphics;
 
 namespace osu.Framework.Testing.Drawables.Steps
@@ -47,7 +48,7 @@ private void checkAssert()
 
         public override string ToString() => "Assert: " + base.ToString();
 
-        private class TracedException : Exception
+        private class TracedException : AssertionException
         {
             private readonly StackTrace trace;
 

osu.Framework/Testing/Drawables/Steps/UntilStepButton.cs

@@ -6,6 +6,7 @@
 using System;
 using System.Diagnostics;
 using System.Text;
+using NUnit.Framework;
 using osu.Framework.Graphics;
 using osuTK.Graphics;
 
@@ -62,7 +63,7 @@ public UntilStepButton(Func<bool> waitUntilTrueDelegate, bool isSetupStep = fals
                     if (getFailureMessage != null)
                         builder.Append($": {getFailureMessage()}");
 
-                    throw new TimeoutException(builder.ToString());
+                    throw new AssertionException(builder.ToString());
                 }
 
                 Action?.Invoke();

osu.Framework/Testing/TestBrowser.cs

@@ -517,8 +517,7 @@ private void finishLoad(TestScene newTest, Action onCompletion)
 
             void addSetUpSteps()
             {
-                var setUpMethods = ReflectionUtils.GetMethodsWithAttribute(newTest.GetType(), typeof(SetUpAttribute), true)
-                                                  .Where(m => m.Name != nameof(TestScene.SetUpTestForNUnit));
+                var setUpMethods = ReflectionUtils.GetMethodsWithAttribute(newTest.GetType(), typeof(SetUpAttribute), true);
 
                 if (setUpMethods.Any())
                 {

osu.Framework/Testing/TestScene.cs

@@ -11,6 +11,7 @@
 using JetBrains.Annotations;
 using NUnit.Framework;
 using NUnit.Framework.Constraints;
+using NUnit.Framework.Interfaces;
 using NUnit.Framework.Internal;
 using osu.Framework.Allocation;
 using osu.Framework.Development;
@@ -32,6 +33,7 @@
 namespace osu.Framework.Testing
 {
     [TestFixture]
+    [UseTestSceneRunner]
     public abstract partial class TestScene : Container
     {
         public readonly FillFlowContainer<Drawable> StepsContainer;
@@ -462,51 +464,8 @@ public void SetupGameHostForNUnit()
             }
         }
 
-        [SetUp]
-        public void SetUpTestForNUnit()
+        internal virtual void RunAfterTest()
         {
-            if (DebugUtils.IsNUnitRunning)
-            {
-                // Since the host is created in OneTimeSetUp, all game threads will have the fixture's execution context
-                // This is undesirable since each test is run using those same threads, so we must make sure the execution context
-                // for the game threads refers to the current _test_ execution context for each test
-                var executionContext = TestExecutionContext.CurrentContext;
-
-                foreach (var thread in host.Threads)
-                {
-                    thread.Scheduler.Add(() =>
-                    {
-                        TestExecutionContext.CurrentContext.CurrentResult = executionContext.CurrentResult;
-                        TestExecutionContext.CurrentContext.CurrentTest = executionContext.CurrentTest;
-                        TestExecutionContext.CurrentContext.CurrentCulture = executionContext.CurrentCulture;
-                        TestExecutionContext.CurrentContext.CurrentPrincipal = executionContext.CurrentPrincipal;
-                        TestExecutionContext.CurrentContext.CurrentRepeatCount = executionContext.CurrentRepeatCount;
-                        TestExecutionContext.CurrentContext.CurrentUICulture = executionContext.CurrentUICulture;
-                    });
-                }
-
-                if (TestContext.CurrentContext.Test.MethodName != nameof(TestConstructor))
-                    schedule(() => StepsContainer.Clear());
-
-                RunSetUpSteps();
-            }
-        }
-
-        [TearDown]
-        public virtual void RunTestsFromNUnit()
-        {
-            RunTearDownSteps();
-
-            checkForErrors();
-            runner.RunTestBlocking(this);
-            checkForErrors();
-
-            if (FrameworkEnvironment.ForceTestGC)
-            {
-                // Force any unobserved exceptions to fire against the current test run.
-                // Without this they could be delayed until a future test scene is running, making tracking down the cause difficult.
-                collectAndFireUnobserved();
-            }
         }
 
         [OneTimeTearDown]
@@ -542,12 +501,6 @@ private void checkForErrors()
                 throw runTask.Exception;
         }
 
-        private static void collectAndFireUnobserved()
-        {
-            GC.Collect();
-            GC.WaitForPendingFinalizers();
-        }
-
         private class TestSceneHost : TestRunHeadlessGameHost
         {
             private readonly Action onExitRequest;
@@ -567,6 +520,62 @@ protected override void PerformExit(bool immediately)
 
             public void ExitFromRunner() => base.PerformExit(false);
         }
+
+        private class UseTestSceneRunnerAttribute : TestActionAttribute
+        {
+            public override void BeforeTest(ITest test)
+            {
+                if (test.Fixture is not TestScene testScene)
+                    return;
+
+                // Since the host is created in OneTimeSetUp, all game threads will have the fixture's execution context
+                // This is undesirable since each test is run using those same threads, so we must make sure the execution context
+                // for the game threads refers to the current _test_ execution context for each test
+                var executionContext = TestExecutionContext.CurrentContext;
+
+                foreach (var thread in testScene.host.Threads)
+                {
+                    thread.Scheduler.Add(() =>
+                    {
+                        TestExecutionContext.CurrentContext.CurrentResult = executionContext.CurrentResult;
+                        TestExecutionContext.CurrentContext.CurrentTest = executionContext.CurrentTest;
+                        TestExecutionContext.CurrentContext.CurrentCulture = executionContext.CurrentCulture;
+                        TestExecutionContext.CurrentContext.CurrentPrincipal = executionContext.CurrentPrincipal;
+                        TestExecutionContext.CurrentContext.CurrentRepeatCount = executionContext.CurrentRepeatCount;
+                        TestExecutionContext.CurrentContext.CurrentUICulture = executionContext.CurrentUICulture;
+                    });
+                }
+
+                if (TestContext.CurrentContext.Test.MethodName != nameof(TestScene.TestConstructor))
+                    testScene.Schedule(() => testScene.StepsContainer.Clear());
+
+                testScene.RunSetUpSteps();
+            }
+
+            public override void AfterTest(ITest test)
+            {
+                if (test.Fixture is not TestScene testScene)
+                    return;
+
+                testScene.RunTearDownSteps();
+
+                testScene.checkForErrors();
+                testScene.runner.RunTestBlocking(testScene);
+                testScene.checkForErrors();
+
+                if (FrameworkEnvironment.ForceTestGC)
+                {
+                    // Force any unobserved exceptions to fire against the current test run.
+                    // Without this they could be delayed until a future test scene is running, making tracking down the cause difficult.
+                    GC.Collect();
+                    GC.WaitForPendingFinalizers();
+                }
+
+                testScene.RunAfterTest();
+            }
+
+            public override ActionTargets Targets => ActionTargets.Test;
+        }
     }
 
     #endregion