Merge pull request #19504 from peppy/skin-serialisation-test-coverfage

Add test coverage of deserialisation of skin layouts

osu.Game.Tests/Skins/SkinDeserialisationTest.cs

@@ -0,0 +1,128 @@
+// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using NUnit.Framework;
+using osu.Framework.Audio.Sample;
+using osu.Framework.Bindables;
+using osu.Framework.Graphics.OpenGL.Textures;
+using osu.Framework.Graphics.Textures;
+using osu.Framework.IO.Stores;
+using osu.Game.Audio;
+using osu.Game.IO;
+using osu.Game.IO.Archives;
+using osu.Game.Screens.Play.HUD;
+using osu.Game.Screens.Play.HUD.HitErrorMeters;
+using osu.Game.Skinning;
+using osu.Game.Tests.Resources;
+
+namespace osu.Game.Tests.Skins
+{
+    /// <summary>
+    /// Test that the main components (which are serialised based on namespace/class name)
+    /// remain compatible with any changes.
+    /// </summary>
+    /// <remarks>
+    /// If this test breaks, check any naming or class structure changes.
+    /// Migration rules may need to be added to <see cref="Skin"/>.
+    /// </remarks>
+    [TestFixture]
+    public class SkinDeserialisationTest
+    {
+        private static readonly string[] available_skins =
+        {
+            // Covers song progress before namespace changes, and most other components.
+            "Archives/modified-default-20220723.osk",
+            "Archives/modified-classic-20220723.osk",
+            // Covers legacy song progress, UR counter, colour hit error metre.
+            "Archives/modified-classic-20220801.osk"
+        };
+
+        /// <summary>
+        /// If this test fails, new test resources should be added to include new components.
+        /// </summary>
+        [Test]
+        public void TestSkinnableComponentsCoveredByDeserialisationTests()
+        {
+            HashSet<Type> instantiatedTypes = new HashSet<Type>();
+
+            foreach (string oskFile in available_skins)
+            {
+                using (var stream = TestResources.OpenResource(oskFile))
+                using (var storage = new ZipArchiveReader(stream))
+                {
+                    var skin = new TestSkin(new SkinInfo(), null, storage);
+
+                    foreach (var target in skin.DrawableComponentInfo)
+                    {
+                        foreach (var info in target.Value)
+                            instantiatedTypes.Add(info.Type);
+                    }
+                }
+            }
+
+            var editableTypes = SkinnableInfo.GetAllAvailableDrawables().Where(t => (Activator.CreateInstance(t) as ISkinnableDrawable)?.IsEditable == true);
+
+            Assert.That(instantiatedTypes, Is.EquivalentTo(editableTypes));
+        }
+
+        [Test]
+        public void TestDeserialiseModifiedDefault()
+        {
+            using (var stream = TestResources.OpenResource("Archives/modified-default-20220723.osk"))
+            using (var storage = new ZipArchiveReader(stream))
+            {
+                var skin = new TestSkin(new SkinInfo(), null, storage);
+
+                Assert.That(skin.DrawableComponentInfo, Has.Count.EqualTo(2));
+                Assert.That(skin.DrawableComponentInfo[SkinnableTarget.MainHUDComponents], Has.Length.EqualTo(9));
+            }
+        }
+
+        [Test]
+        public void TestDeserialiseModifiedClassic()
+        {
+            using (var stream = TestResources.OpenResource("Archives/modified-classic-20220723.osk"))
+            using (var storage = new ZipArchiveReader(stream))
+            {
+                var skin = new TestSkin(new SkinInfo(), null, storage);
+
+                Assert.That(skin.DrawableComponentInfo, Has.Count.EqualTo(2));
+                Assert.That(skin.DrawableComponentInfo[SkinnableTarget.MainHUDComponents], Has.Length.EqualTo(6));
+                Assert.That(skin.DrawableComponentInfo[SkinnableTarget.SongSelect], Has.Length.EqualTo(1));
+
+                var skinnableInfo = skin.DrawableComponentInfo[SkinnableTarget.SongSelect].First();
+
+                Assert.That(skinnableInfo.Type, Is.EqualTo(typeof(SkinnableSprite)));
+                Assert.That(skinnableInfo.Settings.First().Key, Is.EqualTo("sprite_name"));
+                Assert.That(skinnableInfo.Settings.First().Value, Is.EqualTo("ppy_logo-2.png"));
+            }
+
+            using (var stream = TestResources.OpenResource("Archives/modified-classic-20220801.osk"))
+            using (var storage = new ZipArchiveReader(stream))
+            {
+                var skin = new TestSkin(new SkinInfo(), null, storage);
+                Assert.That(skin.DrawableComponentInfo[SkinnableTarget.MainHUDComponents], Has.Length.EqualTo(8));
+                Assert.That(skin.DrawableComponentInfo[SkinnableTarget.MainHUDComponents].Select(i => i.Type), Contains.Item(typeof(UnstableRateCounter)));
+                Assert.That(skin.DrawableComponentInfo[SkinnableTarget.MainHUDComponents].Select(i => i.Type), Contains.Item(typeof(ColourHitErrorMeter)));
+                Assert.That(skin.DrawableComponentInfo[SkinnableTarget.MainHUDComponents].Select(i => i.Type), Contains.Item(typeof(LegacySongProgress)));
+            }
+        }
+
+        private class TestSkin : Skin
+        {
+            public TestSkin(SkinInfo skin, IStorageResourceProvider? resources, IResourceStore<byte[]>? storage = null, string configurationFilename = "skin.ini")
+                : base(skin, resources, storage, configurationFilename)
+            {
+            }
+
+            public override Texture GetTexture(string componentName, WrapMode wrapModeS, WrapMode wrapModeT) => throw new NotImplementedException();
+
+            public override IBindable<TValue> GetConfig<TLookup, TValue>(TLookup lookup) => throw new NotImplementedException();
+
+            public override ISample GetSample(ISampleInfo sampleInfo) => throw new NotImplementedException();
+        }
+    }
+}

osu.Game/Screens/Play/HUD/SkinnableInfo.cs

@@ -98,5 +98,14 @@ namespace osu.Game.Screens.Play.HUD
                 return Drawable.Empty();
             }
         }
+
+        public static Type[] GetAllAvailableDrawables()
+        {
+            return typeof(OsuGame).Assembly.GetTypes()
+                                  .Where(t => !t.IsInterface && !t.IsAbstract)
+                                  .Where(t => typeof(ISkinnableDrawable).IsAssignableFrom(t))
+                                  .OrderBy(t => t.Name)
+                                  .ToArray();
+        }
     }
 }

osu.Game/Skinning/Editor/SkinComponentToolbox.cs

@@ -1,11 +1,8 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
-#nullable disable
-
 using System;
 using System.Diagnostics;
-using System.Linq;
 using osu.Framework.Allocation;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
@@ -16,24 +13,25 @@ using osu.Game.Graphics.Sprites;
 using osu.Game.Graphics.UserInterface;
 using osu.Game.Overlays;
 using osu.Game.Screens.Edit.Components;
+using osu.Game.Screens.Play.HUD;
 using osuTK;
 
 namespace osu.Game.Skinning.Editor
 {
     public class SkinComponentToolbox : EditorSidebarSection
     {
-        public Action<Type> RequestPlacement;
+        public Action<Type>? RequestPlacement;
 
-        private readonly CompositeDrawable target;
+        private readonly CompositeDrawable? target;
 
-        public SkinComponentToolbox(CompositeDrawable target = null)
+        private FillFlowContainer fill = null!;
+
+        public SkinComponentToolbox(CompositeDrawable? target = null)
             : base("Components")
         {
             this.target = target;
         }
 
-        private FillFlowContainer fill;
-
         [BackgroundDependencyLoader]
         private void load()
         {
@@ -52,12 +50,7 @@ namespace osu.Game.Skinning.Editor
         {
             fill.Clear();
 
-            var skinnableTypes = typeof(OsuGame).Assembly.GetTypes()
-                                                .Where(t => !t.IsInterface && !t.IsAbstract)
-                                                .Where(t => typeof(ISkinnableDrawable).IsAssignableFrom(t))
-                                                .OrderBy(t => t.Name)
-                                                .ToArray();
-
+            var skinnableTypes = SkinnableInfo.GetAllAvailableDrawables();
             foreach (var type in skinnableTypes)
                 attemptAddComponent(type);
         }
@@ -90,21 +83,21 @@ namespace osu.Game.Skinning.Editor
 
         public class ToolboxComponentButton : OsuButton
         {
+            public Action<Type>? RequestPlacement;
+
             protected override bool ShouldBeConsideredForInput(Drawable child) => false;
 
             public override bool PropagateNonPositionalInputSubTree => false;
 
             private readonly Drawable component;
-            private readonly CompositeDrawable dependencySource;
+            private readonly CompositeDrawable? dependencySource;
 
-            public Action<Type> RequestPlacement;
-
-            private Container innerContainer;
+            private Container innerContainer = null!;
 
             private const float contracted_size = 60;
             private const float expanded_size = 120;
 
-            public ToolboxComponentButton(Drawable component, CompositeDrawable dependencySource)
+            public ToolboxComponentButton(Drawable component, CompositeDrawable? dependencySource)
             {
                 this.component = component;
                 this.dependencySource = dependencySource;
@@ -184,9 +177,9 @@ namespace osu.Game.Skinning.Editor
 
         public class DependencyBorrowingContainer : Container
         {
-            private readonly CompositeDrawable donor;
+            private readonly CompositeDrawable? donor;
 
-            public DependencyBorrowingContainer(CompositeDrawable donor)
+            public DependencyBorrowingContainer(CompositeDrawable? donor)
             {
                 this.donor = donor;
             }