Merge pull request #2234 from andy840119/add-check-test-assert-usage

Add more strict check about test assert usage and naming.

osu.Game.Rulesets.Karaoke.Architectures/Edit/Checks/TestCheck.cs

@@ -34,26 +34,26 @@ public void CheckCheckClassNamingAndInherit()
 
             foreach (var checkClass in issues)
             {
-                Assert.True(checkClass.InheritedClasses.Contains(baseIssue), $"Class inherit is invalid: {checkClass}");
-                Assert.True(checkClass.NameEndsWith("Issue"), $"Class name is invalid: {checkClass}");
+                Assert.IsTrue(checkClass.InheritedClasses.Contains(baseIssue), $"Class inherit is invalid: {checkClass}");
+                Assert.IsTrue(checkClass.NameEndsWith("Issue"), $"Class name is invalid: {checkClass}");
             }
 
             // checks
             Assert.NotZero(checks.Length, $"{nameof(ICheck)} amount is weird.");
 
             foreach (var check in checks)
             {
-                Assert.True(check.ImplementsInterface(baseCheck), $"Class inherit is invalid: {check}");
-                Assert.True(check.NameStartsWith("Check"), $"Class name is invalid: {check}");
+                Assert.IsTrue(check.ImplementsInterface(baseCheck), $"Class inherit is invalid: {check}");
+                Assert.IsTrue(check.NameStartsWith("Check"), $"Class name is invalid: {check}");
             }
 
             // issue templates.
             Assert.NotZero(issueTemplates.Length, $"{nameof(IssueTemplate)} amount is weird");
 
             foreach (var checkClass in issueTemplates)
             {
-                Assert.True(checkClass.InheritedClasses.Contains(baseIssueTemplate), $"Class inherit is invalid: {checkClass}");
-                Assert.True(checkClass.NameStartsWith("IssueTemplate"), $"Class name is invalid: {checkClass}");
+                Assert.IsTrue(checkClass.InheritedClasses.Contains(baseIssueTemplate), $"Class inherit is invalid: {checkClass}");
+                Assert.IsTrue(checkClass.NameStartsWith("IssueTemplate"), $"Class name is invalid: {checkClass}");
             }
         });
     }

osu.Game.Rulesets.Karaoke.Architectures/Edit/Checks/TestCheckTest.cs

@@ -5,6 +5,7 @@
 using ArchUnitNET.Domain;
 using ArchUnitNET.Domain.Extensions;
 using NUnit.Framework;
+using osu.Game.Rulesets.Edit.Checks.Components;
 using osu.Game.Rulesets.Karaoke.Tests.Editor.Checks;
 
 namespace osu.Game.Rulesets.Karaoke.Architectures.Edit.Checks;
@@ -47,9 +48,10 @@ public void CheckShouldContainsTest()
 
     [Test]
     [Project.KaraokeTest(true)]
-    public void CheckTestMethod()
+    public void CheckTestClassAndMethod()
     {
-        var architecture = GetProjectArchitecture();
+        var architecture = GetProjectArchitecture(new Project.KaraokeAttribute());
+        var baseCheck = architecture.GetInterfaceOfType(typeof(ICheck));
         var baseCheckTest = architecture.GetClassOfType(typeof(BaseCheckTest<>));
 
         var assertOkMethod = baseCheckTest.GetMethodMembersContainsName("AssertOk").FirstOrDefault();
@@ -66,17 +68,41 @@ public void CheckTestMethod()
 
             foreach (var checkTest in allCheckTests)
             {
+                // check the class naming.
+                Assert.IsTrue(isTestClassValid(checkTest, baseCheck), $"Test class {checkTest} should have correct naming");
+
+                // check the test method naming in the test case.
                 var testMethods = checkTest.GetAllTestMembers(architecture).ToArray();
-                Assert.NotZero(testMethods.Length, $"No test method  in the {checkTest}");
+                Assert.NotZero(testMethods.Length, $"No test method in the {checkTest}");
 
                 foreach (var testMethod in testMethods)
                 {
+                    Assert.IsTrue(isTestNamingValid(testMethod), $"Test method {testMethod} should have correct naming");
                     Assert.IsTrue(isTestMethod(testMethod), $"Test method {testMethod} should call {assertOkMethod} or {assertNotOkMethod} method.");
                 }
             }
 
             return;
 
+            static bool isTestClassValid(Class testClass, Interface baseCheck)
+            {
+                var testCheck = testClass.GetGenericTypes().OfType<Class>().First(x => x.ImplementsInterface(baseCheck));
+                return testClass.NameStartsWith(testCheck.Name);
+            }
+
+            static bool isTestNamingValid(IMember testMethod)
+            {
+                var calledMethods = testMethod.GetMethodCallDependencies().FirstOrDefault(x => x.TargetMember.NameStartsWith("AssertNotOk"));
+
+                if (calledMethods != null)
+                {
+                    // todo: should get the generic type from the AssertNotOk method. the end of the method should be the issue template.
+                    return testMethod.NameStartsWith("TestCheck");
+                }
+
+                return true;
+            }
+
             static bool isTestMethod(IMember testMethod)
             {
                 var calledMethods = testMethod.GetCalledMethods().ToArray();

osu.Game.Rulesets.Karaoke.Architectures/Extensions.cs

@@ -6,14 +6,42 @@
 using System.Linq;
 using System.Text.RegularExpressions;
 using ArchUnitNET.Domain;
+using ArchUnitNET.Domain.Dependencies;
 using ArchUnitNET.Domain.Extensions;
 using NUnit.Framework;
+using osu.Game.Rulesets.Karaoke.Tests;
 using Attribute = ArchUnitNET.Domain.Attribute;
 
 namespace osu.Game.Rulesets.Karaoke.Architectures;
 
 public static class Extensions
 {
+    #region Architecture
+
+    public static IEnumerable<Class> GetAllTestClass(this Architecture architecture)
+    {
+        var testProject = new Project.KaraokeTestAttribute();
+        var testClasses = architecture.Classes.Where(x =>
+                                      {
+                                          if (x.Namespace.RelativeNameStartsWith(testProject, "Helper"))
+                                              return false;
+
+                                          return x.Namespace.RelativeNameStartsWith(testProject, "");
+                                      })
+                                      .Except(new[]
+                                      {
+                                          architecture.GetClassOfType(typeof(KaraokeTestBrowser)),
+                                          architecture.GetClassOfType(typeof(VisualTestRunner)),
+                                      }).ToArray();
+
+        if (testClasses.Length == 0)
+            throw new InvalidOperationException("No test class found in the project.");
+
+        return testClasses;
+    }
+
+    #endregion
+
     #region Class
 
     public static IEnumerable<Class> GetInnerClasses(this Class @class)
@@ -40,6 +68,11 @@ public static bool HasAttributeInSelfOrChild(this Class @class, Attribute attrib
 
     #region Name
 
+    public static IEnumerable<IType> GetGenericTypes(this Class @class)
+    {
+        return @class.GetInheritsBaseClassDependencies().SelectMany(x => x.TargetGenericArguments.Select(arg => arg.Type));
+    }
+
     public static bool RelativeNameStartsWith(
         this IHasName cls,
         Project.ProjectAttribute project,

osu.Game.Rulesets.Karaoke.Architectures/TestClass.cs

@@ -42,7 +42,7 @@ public void CheckAbstractClassLocation()
                 if (allChildClasses.Length == 0)
                     continue;
 
-                Assert.True(isAllowAbstractClassPosition(abstractClass, allChildClasses), $"Those child class: \n{string.Join('\n', allChildClasses.Select(x => x.ToString()))}\n\n is not in the child namespace of: \n{abstractClass}");
+                Assert.IsTrue(isAllowAbstractClassPosition(abstractClass, allChildClasses), $"Those child class: \n{string.Join('\n', allChildClasses.Select(x => x.ToString()))}\n\n is not in the child namespace of: \n{abstractClass}");
             }
         });
         return;

osu.Game.Rulesets.Karaoke.Architectures/TestTestClass.cs

@@ -1,7 +1,9 @@
 // Copyright (c) andy840119 <[email protected]>. Licensed under the GPL Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using System.Collections.Generic;
 using System.Linq;
+using ArchUnitNET.Domain;
 using ArchUnitNET.Domain.Extensions;
 using NUnit.Framework;
 using osu.Framework.Testing;
@@ -32,23 +34,10 @@ public void CheckTestClassAndMethodNaming()
         var headlessTestScene = architecture.GetAttributeOfType(typeof(HeadlessTestAttribute));
 
         // get all test classes
-        var testClasses = architecture.Classes.Where(x =>
-                                      {
-                                          if (x.Namespace.RelativeNameStartsWith(GetExecuteProject(), "Helper"))
-                                              return false;
-
-                                          return x.Namespace.RelativeNameStartsWith(GetExecuteProject(), "");
-                                      })
-                                      .Except(new[]
-                                      {
-                                          architecture.GetClassOfType(typeof(KaraokeTestBrowser)),
-                                          architecture.GetClassOfType(typeof(VisualTestRunner)),
-                                      }).ToArray();
+        var testClasses = architecture.GetAllTestClass().ToArray();
 
         Assertion(() =>
         {
-            Assert.NotZero(testClasses.Length, "No test class found");
-
             foreach (var testClass in testClasses)
             {
                 var testMethods = testClass.GetAllTestMembers(architecture).ToArray();
@@ -88,4 +77,25 @@ public void CheckTestClassAndMethodNaming()
             }
         });
     }
+
+    [Test]
+    [Project.KaraokeTest(true)]
+    public void CheckAssertion()
+    {
+        var architecture = GetProjectArchitecture();
+        var tests = architecture.GetAllTestClass();
+        var assert = architecture.GetClassOfType(typeof(Assert));
+
+        Assertion(() =>
+        {
+            foreach (var test in tests)
+            {
+                var assertions = test.GetCalledMethods().Where(x => EqualityComparer<IType>.Default.Equals(x.DeclaringType, assert)).ToArray();
+
+                // Assertion.
+                Assert.IsFalse(assertions.Any(x => x.NameStartsWith("True")), $"Should use {nameof(Assert.IsTrue)} instead of {nameof(Assert.True)} in the {test}.");
+                Assert.IsFalse(assertions.Any(x => x.NameStartsWith("False")), $"Should use {nameof(Assert.IsFalse)} instead of {nameof(Assert.False)} in the {test}.");
+            }
+        });
+    }
 }

osu.Game.Rulesets.Karaoke.Tests/Editor/Checks/CheckBeatmapAvailableTranslatesTest.cs

@@ -49,7 +49,20 @@ public void TestHaveLyricButNoLanguage()
     }
 
     [Test]
-    public void TestHaveLyricAndLanguage()
+    public void TestEveryLyricContainsTranslate()
+    {
+        var translateLanguages = new List<CultureInfo> { new("Ja-jp") };
+        var beatmap = createTestingBeatmap(translateLanguages, new[]
+        {
+            createLyric(new CultureInfo("Ja-jp"), "translate1"),
+            createLyric(new CultureInfo("Ja-jp"), "translate2"),
+        });
+
+        AssertOk(getContext(beatmap));
+    }
+
+    [Test]
+    public void TestCheckMissingTranslate()
     {
         // no lyric with translate string. (should have issue)
         var translateLanguages = new List<CultureInfo> { new("Ja-jp") };
@@ -75,39 +88,30 @@ public void TestHaveLyricAndLanguage()
             createLyric(new CultureInfo("Ja-jp"), string.Empty),
         });
         AssertNotOk<IssueTemplateMissingTranslate>(getContext(beatmap3));
+    }
 
+    [Test]
+    public void TestCheckMissingPartialTranslate()
+    {
         // some lyric with translate string. (should have issue)
+        var translateLanguages = new List<CultureInfo> { new("Ja-jp") };
         var beatmap4 = createTestingBeatmap(translateLanguages, new[]
         {
             createLyric(new CultureInfo("Ja-jp"), "translate1"),
             createLyric(new CultureInfo("Ja-jp")),
         });
         AssertNotOk<IssueTemplateMissingPartialTranslate>(getContext(beatmap4));
+    }
 
-        // every lyric with translate string. (should not have issue)
-        var beatmap5 = createTestingBeatmap(translateLanguages, new[]
-        {
-            createLyric(new CultureInfo("Ja-jp"), "translate1"),
-            createLyric(new CultureInfo("Ja-jp"), "translate2"),
-        });
-        AssertOk(getContext(beatmap5));
-
+    [Test]
+    public void TestCheckContainsNotListedLanguage()
+    {
         // lyric translate not listed. (should have issue)
         var beatmap6 = createTestingBeatmap(null, new[]
         {
             createLyric(new CultureInfo("en-US"), "translate1"),
         });
         AssertNotOk<IssueTemplateContainsNotListedLanguage>(getContext(beatmap6));
-
-        static Lyric createLyric(CultureInfo? cultureInfo = null, string translate = null!)
-        {
-            var lyric = new Lyric();
-            if (cultureInfo == null)
-                return lyric;
-
-            lyric.Translates.Add(cultureInfo, translate);
-            return lyric;
-        }
     }
 
     private static IBeatmap createTestingBeatmap(List<CultureInfo>? translateLanguage, IEnumerable<Lyric>? lyrics)
@@ -126,4 +130,14 @@ private static IBeatmap createTestingBeatmap(List<CultureInfo>? translateLanguag
 
     private static BeatmapVerifierContext getContext(IBeatmap beatmap)
         => new(beatmap, new TestWorkingBeatmap(beatmap));
+
+    private static Lyric createLyric(CultureInfo? cultureInfo = null, string translate = null!)
+    {
+        var lyric = new Lyric();
+        if (cultureInfo == null)
+            return lyric;
+
+        lyric.Translates.Add(cultureInfo, translate);
+        return lyric;
+    }
 }

osu.Game.Rulesets.Karaoke.Tests/Editor/Checks/CheckBeatmapStageInfoTest.cs

@@ -19,7 +19,7 @@
 
 namespace osu.Game.Rulesets.Karaoke.Tests.Editor.Checks;
 
-public class CheckBeatmapStageInfoTest : BeatmapPropertyCheckTest<CheckBeatmapStageInfoTest.TestCheckBeatmapStageInfo>
+public class CheckBeatmapStageInfoTest : BeatmapPropertyCheckTest<CheckBeatmapStageInfoTest.CheckBeatmapStageInfo>
 {
     [Test]
     public void TestCheckNoElement()
@@ -63,11 +63,11 @@ public void TestCheckMappingItemNotExist()
         AssertNotOk<IssueTemplateMappingItemNotExist>(getContext(beatmap));
     }
 
-    public class TestCheckBeatmapStageInfo : CheckBeatmapStageInfo<ClassicStageInfo>
+    public class CheckBeatmapStageInfo : CheckBeatmapStageInfo<ClassicStageInfo>
     {
         protected override string Description => "Checks for testing the shared logic";
 
-        public TestCheckBeatmapStageInfo()
+        public CheckBeatmapStageInfo()
         {
             // Note that we only test the lyric layout category.
             RegisterCategory(x => x.StyleCategory, 0);