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 #6418

Merged
merged 5 commits into from
Nov 12, 2024

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.
    • Primary reason I did this is because I found ToggleStepButton missing the isSetupStep param. It feels like the contracts were too loose being optional ctor params.
  • UntilStepButton now has a callstack similar to AssertButton.
  • AddStep and AddLabelStep no longer return the step button. It's not used in o!f and osu!, and I'm not sure what practice use it could ever have.
  • Every method (e.g. AddUntilStep) calls the AddStep(StepButton) method which does the schedule + add to container. In particular, this is important for generating correct callstacks as they're currently generated inside the lambda and lose context. This 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

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems okay, although before merge I'll probably run this against game-side tests just to make sure nothing funny happens

@bdach
Copy link
Collaborator

bdach commented Nov 12, 2024

Game-side tests passed ok, so here goes...

@bdach bdach enabled auto-merge November 12, 2024 09:56
@bdach bdach disabled auto-merge November 12, 2024 10:12
@bdach bdach merged commit 6b90d0c into ppy:master Nov 12, 2024
12 of 13 checks passed
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.

2 participants