Skip to content
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

Refactor step buttons + improve assertion messages #6417

Closed
wants to merge 5 commits into from

Conversation

smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Nov 11, 2024

I got annoyed with totally useless test failure messages and having to follow the steps to the failures.

A few important changes here:

  • I've removed StepButton constructors except where required (UntilStepButton, RepeatStepButton) to preserve behaviour. They now use properties including required and { get; init; } keywords to indicate expected uses.
  • UntilStepButton now has a callstack.
  • AddStep and AddLabelStep no longer returns the step button. It's not used in o!f and osu!, and I'm not sure where it could ever find a use in practice.
  • Every method (e.g. AddUntilStep) calls into the AddStep(StepButton) method which does the scheduling. This is extremely important for generating correct callstacks - it is currently generated inside the lambda which loses context and is why our traces are terrible.

If deemed necessary, I can split this PR up into two. I kinda went down the rabbit hole to see where it ends, so it combines the refactoring.

Demo:

public class SomeTestScene : TestScene
{
    [Test]
    public void FailingAssertStep()
    {
        AddAssert("fail", () => false);
    }

    [Test]
    public void PassingAssertStep()
    {
        AddAssert("pass", () => true);
    }

    [Test]
    public void FailingUntilStep()
    {
        AddUntilStep("fail", () => false);
    }

    [Test]
    public void PassingUntilStep()
    {
        AddUntilStep("pass", () => true);
    }

    [Test]
    public void RepeatStep()
    {
        AddRepeatStep("repeat", () => { }, 10);
    }

    [Test]
    public void WaitStep()
    {
        AddWaitStep("wait", 10);
    }
}

Before:

  Failed FailingAssertStep [81 ms]
  Error Message:
   fail
  Stack Trace:
     at osu.Framework.Threading.ScheduledDelegate.InvokeTask()
   at osu.Framework.Threading.ScheduledDelegate.RunTaskInternal()
   at osu.Framework.Threading.Scheduler.Update()
   at osu.Framework.Graphics.Drawable.UpdateSubTree()
   at osu.Framework.Graphics.Containers.CompositeDrawable.UpdateSubTree()
   at osu.Framework.Graphics.Containers.CompositeDrawable.updateChild(Drawable c)
   at osu.Framework.Graphics.Containers.CompositeDrawable.UpdateSubTree()
   at osu.Framework.Graphics.Containers.CompositeDrawable.updateChild(Drawable c)
   at osu.Framework.Graphics.Containers.CompositeDrawable.UpdateSubTree()
   at osu.Framework.Graphics.Containers.CompositeDrawable.updateChild(Drawable c)
   at osu.Framework.Graphics.Containers.CompositeDrawable.UpdateSubTree()
   at osu.Framework.Graphics.Containers.CompositeDrawable.updateChild(Drawable c)
   at osu.Framework.Graphics.Containers.CompositeDrawable.UpdateSubTree()
   at osu.Framework.Graphics.Containers.CompositeDrawable.updateChild(Drawable c)
   at osu.Framework.Graphics.Containers.CompositeDrawable.UpdateSubTree()
   at osu.Framework.Graphics.Containers.CompositeDrawable.updateChild(Drawable c)
   at osu.Framework.Graphics.Containers.CompositeDrawable.UpdateSubTree()
   at osu.Framework.Graphics.Containers.CompositeDrawable.updateChild(Drawable c)
   at osu.Framework.Graphics.Containers.CompositeDrawable.UpdateSubTree()
   at osu.Framework.Platform.GameHost.UpdateFrame()
   at osu.Framework.Platform.HeadlessGameHost.UpdateFrame()
   at osu.Framework.Threading.GameThread.processFrame()
   at osu.Framework.Threading.GameThread.RunSingleFrame()
   at osu.Framework.Threading.GameThread.<createThread>g__runWork|70_0()
   at System.Threading.Thread.StartHelper.Callback(Object state)

---

  Failed FailingUntilStep [10 s]
  Error Message:
   "fail" timed out
  Stack Trace:
     at osu.Framework.Testing.Drawables.Steps.UntilStepButton.<>c__DisplayClass11_0.<.ctor>b__0() in /Users/smgi/Repos/osu-framework/osu.Framework/Testing/Drawables/Steps/UntilStepButton.cs:line 66
   at osu.Framework.Testing.Drawables.Steps.StepButton.PerformStep(Boolean userTriggered) in /Users/smgi/Repos/osu-framework/osu.Framework/Testing/Drawables/Steps/StepButton.cs:line 124
   at osu.Framework.Testing.TestScene.runNextStep(Action onCompletion, Action`1 onError, Func`2 stopCondition) in /Users/smgi/Repos/osu-framework/osu.Framework/Testing/TestScene.cs:line 235
--- End of stack trace from previous location ---
   at osu.Framework.Testing.TestSceneTestRunner.TestRunner.RunTestBlocking(TestScene test) in /Users/smgi/Repos/osu-framework/osu.Framework/Testing/TestSceneTestRunner.cs:line 89
   at osu.Framework.Testing.TestSceneTestRunner.RunTestBlocking(TestScene test) in /Users/smgi/Repos/osu-framework/osu.Framework/Testing/TestSceneTestRunner.cs:line 32
   at osu.Framework.Testing.TestScene.UseTestSceneRunnerAttribute.AfterTest(ITest test) in /Users/smgi/Repos/osu-framework/osu.Framework/Testing/TestScene.cs:line 564
   at NUnit.Framework.Internal.Commands.TestActionCommand.<>c__DisplayClass0_0.<.ctor>b__1(TestExecutionContext context)
   at NUnit.Framework.Internal.Commands.BeforeAndAfterTestCommand.<>c__DisplayClass1_0.<Execute>b__1()
   at NUnit.Framework.Internal.Commands.DelegatingTestCommand.RunTestMethodInThreadAbortSafeZone(TestExecutionContext context, Action action)

After:

  Failed FailingAssertStep [74 ms]
  Error Message:
   fail
  Stack Trace:
     at osu.Framework.Tests.SomeTestScene.FailingAssertStep() in /Users/smgi/Repos/osu-framework/osu.Framework.Tests/SomeTestScene.cs:line 14

---

  Failed FailingUntilStep [10 s]
  Error Message:
   "fail" timed out
  Stack Trace:
     at osu.Framework.Tests.SomeTestScene.FailingUntilStep() in /Users/smgi/Repos/osu-framework/osu.Framework.Tests/SomeTestScene.cs:line 26

@smoogipoo smoogipoo force-pushed the improve-test-messages branch from cd53fd9 to 43fdea2 Compare November 11, 2024 16:07
@smoogipoo smoogipoo closed this Nov 11, 2024
@peppy peppy self-requested a review November 11, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant