Unit Tests: Coverage Reporting and Github Actions Integration (Funkin…

…Crew#131)

* Initial test suite

* Fix some build warnings

* Implemented working unit tests with coverage

* Reduced some warnings

* Fix a mac-specific issue

* Add 2 additional unit test classes.

* Multiple new unit tests

* Some fixins

* Remove auto-generated file

* WIP on hiding ignored tests

* Added list of debug hotkeys

* Remove old website

* Remove empty file

* Add more unit tests

* Fix bug where arrows would nudge BF

* Fix bug where ctrl/alt would flash capsules

* Fixed bug where bf-old easter egg broke

* Remove duplicate lines

* More test-related stuff

* Some code cleanup

* Add mocking and a test assets folder

* More TESTS!

* Update Hmm...

* Update artist on Monster

* More minor fixes to individual functions

* 1.38% unit test coverage!

* Even more tests? :O

* More unit test work

* Rework migration for BaseRegistry

* gameover fix

* Fix an issue with Lime

* Fix issues with version parsing on data files

* 100 total unit tests!

* Added even MORE unit tests!

* Additional test tweaks :3

* Fixed tests on windows by updating libraries.

* A bunch of smaller syntax tweaks.

* New crash handler catches and logs critical errors!

* Chart editor now has null safety enabled.

* Null safety on all tests

* New Level data test

* Generate proper code coverage reports!

* Disable null safety on ChartEditorState for unit testing

* Update openfl to use latest fixes for crash reporting

* Added unit test to Github Workflow

* Updated unit tests to compile with null safety enabled by inlining assertions.

* Added coverage gutters as a recommended extension

* Impreovements to tests involving exceptions

* Disable a few incomplete tests.

* Add scripts for building unit coverage reports on linux

---------

Co-authored-by: Cameron Taylor <cameron.taylor.ninja@gmail.com>

.github/workflows/build-shit.yml

@@ -58,3 +58,18 @@ jobs:
           butler-key: ${{ secrets.BUTLER_API_KEY}}
           build-dir: export/debug/windows/bin
           target: win
+  test-unit-win:
+    needs: create-nightly-win
+    runs-on: windows-latest
+    permissions:
+       contents: write
+       actions: write
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          submodules: 'recursive'
+      - uses: ./.github/actions/setup-haxeshit
+      - name: Run unit tests
+        run: |
+          cd ./tests/unit/
+          ./start-win-native.bat

.vscode/extensions.json

@@ -6,6 +6,7 @@
     "vshaxe.hxcpp-debugger", // CPP debugging
     "openfl.lime-vscode-extension", // Lime integration
     "esbenp.prettier-vscode", // JSON formatting
-    "redhat.vscode-xml" // XML formatting
+    "redhat.vscode-xml", // XML formatting
+    "ryanluker.vscode-coverage-gutters" // Highlight code coverage
   ]
 }

.vscode/settings.json

@@ -117,5 +117,12 @@
       "args": ["-debug", "-watch"]
     }
   ],
-  "cmake.configureOnOpen": false
+  "cmake.configureOnOpen": false,
+  "coverage-gutters.coverageFileNames": [
+    "lcov.info",
+    "cov.xml",
+    "coverage.xml",
+    "jacoco.xml",
+    "coverage.cobertura.xml"
+  ]
 }

source/funkin/ui/debug/charting/ChartEditorDialogHandler.hx

@@ -257,7 +257,7 @@ class ChartEditorDialogHandler
    * @param closable Whether the dialog can be closed by the user.
    * @return The dialog that was opened.
    */
-  @:haxe.warning("-WVarInit")
+  @:haxe.warning("-WVarInit") // Hide the warning about the onDropFile handler.
   public static function openUploadInstDialog(state:ChartEditorState, closable:Bool = true):Dialog
   {
     var dialog:Dialog = openDialog(state, CHART_EDITOR_DIALOG_UPLOAD_INST_LAYOUT, true, closable);

source/funkin/ui/debug/charting/ChartEditorState.hx

@@ -85,8 +85,8 @@ using Lambda;
  * @author MasterEric
  */
 // Give other classes access to private instance fields
+// @:nullSafety(Loose) // Enable this while developing, then disable to keep unit tests functional!
 
-@:nullSafety
 @:allow(funkin.ui.debug.charting.ChartEditorCommand)
 @:allow(funkin.ui.debug.charting.ChartEditorDialogHandler)
 @:allow(funkin.ui.debug.charting.ChartEditorThemeHandler)

source/funkin/util/macro/InlineMacro.hx

@@ -0,0 +1,37 @@
+package funkin.util.macro;
+
+#if macro
+using funkin.util.tools.ArrayTools;
+#end
+
+/**
+ * A macro to make fields inline.
+ */
+class InlineMacro
+{
+  /**
+   * For the given class, find the (static?) field with the given name and make it inline.
+   * @param field
+   * @param isStatic
+   */
+  public static macro function makeInline(field:String, isStatic:Bool = false):Array<haxe.macro.Expr.Field>
+  {
+    var pos:haxe.macro.Expr.Position = haxe.macro.Context.currentPos();
+    // The FlxBasic class. We can add new properties to this class.
+    var cls:haxe.macro.Type.ClassType = haxe.macro.Context.getLocalClass().get();
+    // The fields of the FlxClass.
+    var fields:Array<haxe.macro.Expr.Field> = haxe.macro.Context.getBuildFields();
+
+    // Find the field with the given name.
+    var targetField:Null<haxe.macro.Expr.Field> = fields.find(function(f) return f.name == field
+      && (MacroUtil.isFieldStatic(f) == isStatic));
+
+    // If the field was not found, throw an error.
+    if (targetField == null) haxe.macro.Context.error("Field " + field + " not found in class " + cls.name, pos);
+
+    // Add the inline access modifier to the field.
+    targetField.access.push(AInline);
+
+    return fields;
+  }
+}

source/funkin/util/macro/MacroUtil.hx

@@ -99,6 +99,11 @@ class MacroUtil
     return null;
   }
 
+  public static function isFieldStatic(field:haxe.macro.Expr.Field):Bool
+  {
+    return field.access.contains(AStatic);
+  }
+
   /**
    * Converts a value to an equivalent macro expression.
    */

tests/unit/README.md

@@ -48,3 +48,9 @@ There are two parameters:
 ### `testDestroy()`
 
 `testDestroy()` tests whether an `IFlxDestroyable` can safely be `destroy()`ed more than once (null reference errors are fairly common here). For this, `destroyable` has to be set during `before()` of the test class.
+
+### Null Safety
+
+Append each test class with `@:nullSafety` to prevent crash bugs while developing.
+
+Note that `Assert.isNotNull(target)` is considered a vlid

tests/unit/assets/preload/data/levels/blankpathtest.json

@@ -0,0 +1,47 @@
+{
+  "version": "1.0.0",
+  "name": "SHOULD FAIL TO PARSE",
+  "titleAsset": "",
+  "props": [
+    {
+      "assetPath": "storymenu/props/gf",
+      "scale": 1.0,
+      "danceEvery": 2,
+      "offsets": [80, 80],
+      "animations": [
+        {
+          "name": "danceLeft",
+          "prefix": "idle0",
+          "frameIndices": [30, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14]
+        },
+        {
+          "name": "danceRight",
+          "prefix": "idle0",
+          "frameIndices": [
+            15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29
+          ]
+        }
+      ]
+    },
+    {
+      "assetPath": "storymenu/props/bf",
+      "scale": 1.0,
+      "danceEvery": 2,
+      "offsets": [150, 80],
+      "animations": [
+        {
+          "name": "idle",
+          "prefix": "idle0",
+          "frameRate": 24
+        },
+        {
+          "name": "confirm",
+          "prefix": "confirm0",
+          "frameRate": 24
+        }
+      ]
+    }
+  ],
+  "background": "#F9CF51",
+  "songs": ["tutorial"]
+}

tests/unit/project.xml

@@ -37,6 +37,9 @@
 
 	<!-- This macro allows addition of new functionality to existing Flixel. -->
 	<haxeflag name="--macro" value="addMetadata('@:build(funkin.util.macro.FlxMacro.buildFlxBasic())', 'flixel.FlxBasic')" />
+	<!-- Macros to satisfy null safety (null safety can't check nested functions, so assertions must be inlined) -->
+	<haxeflag name="--macro" value="addMetadata('@:build(funkin.util.macro.InlineMacro.makeInline(\'fail\', true))', 'massive.munit.Assert')" />
+	<haxeflag name="--macro" value="addMetadata('@:build(funkin.util.macro.InlineMacro.makeInline(\'isNotNull\', true))', 'massive.munit.Assert')" />
 
 	<!-- Assets -->
 	<assets path="assets/preload" rename="assets" exclude="*.ogg" if="web" />
@@ -80,12 +83,22 @@
 	<!-- Test defines -->
 	<set name="no-custom-backend" />
 	<set name="unit-test" />
-	<!--<haxedef name="no-inline" />-->
 	<haxedef name="FLX_UNIT_TEST" />
 	<haxedef name="FLX_RECORD" />
 
-	<!-- Manually set up code coverage -->
+	<!-- Clean up the output -->
+	<haxedef name="no-traces" />
+	<!--
+	-->
+	<haxedef name="ignore-inline" />
+	<haxeflag name="-w" value="-WDeprecated" />
+
+	<!-- Manually set up code coverage (because munit report and lime test are mutually exclusive) -->
+	<haxeflag name="--macro" value="mcover.MCover.coverage(['funkin'],['../../source', 'source/'],[''])" />
 	<haxelib name="mcover" />
 	<haxedef name="MCOVER" />
-	<haxeflag name="--macro" value="mcover.MCover.coverage(['funkin'],['../../source', 'source/'],[''])" />
+	<haxedef name="safeMode"/>
+	<haxedef name="HXCPP_CHECK_POINTER" />
+	<haxedef name="HXCPP_STACK_LINE" />
+	<haxedef name="HXCPP_STACK_TRACE" />
 </project>

tests/unit/report-linux.sh

@@ -0,0 +1,4 @@
+#!/bin/bash
+
+cd ./report/
+genhtml -o ./html/ ./lcov.info

tests/unit/source/FunkinAssert.hx

@@ -10,6 +10,7 @@ using flixel.util.FlxArrayUtil;
 /**
  * @see https://github.com/HaxeFlixel/flixel/tree/dev/tests/unit
  */
+@:nullSafety
 class FunkinAssert
 {
   /**
@@ -21,15 +22,17 @@ class FunkinAssert
    * @param margin The allowed margin of error between the expected and actual values.
    * @param info Info on the position this function was called from. Magic value, passed automatically.
    */
-  public static function areNear(expected:Float, actual:Float, margin:Float = 0.001, ?info:PosInfos):Void
+  public static function areNear(expected:Float, ?actual:Float, margin:Float = 0.001, ?info:PosInfos):Void
   {
+    if (actual == null) Assert.fail('Value [$actual] is null, and cannot be compared to [$expected]', info);
     if (areNearHelper(expected, actual)) Assert.assertionCount++;
     else
       Assert.fail('Value [$actual] is not within [$margin] of [$expected]', info);
   }
 
-  public static function rectsNear(expected:FlxRect, actual:FlxRect, margin:Float = 0.001, ?info:PosInfos):Void
+  public static function rectsNear(expected:FlxRect, ?actual:FlxRect, margin:Float = 0.001, ?info:PosInfos):Void
   {
+    if (actual == null) Assert.fail('Value [$actual] is null, and cannot be compared to [$expected]', info);
     var areNear = areNearHelper(expected.x, actual.x, margin)
       && areNearHelper(expected.y, actual.y, margin)
       && areNearHelper(expected.width, actual.width, margin)
@@ -45,33 +48,83 @@ class FunkinAssert
     return actual >= expected - margin && actual <= expected + margin;
   }
 
-  public static function arraysEqual<T>(expected:Array<T>, actual:Array<T>, ?info:PosInfos):Void
+  public static function arraysEqual<T>(expected:Array<T>, ?actual:Array<T>, ?info:PosInfos):Void
   {
+    if (actual == null) Assert.fail('Value [$actual] is null, and cannot be compared to [$expected]', info);
     if (expected.equals(actual)) Assert.assertionCount++;
     else
       Assert.fail('\nExpected\n   ${expected}\nbut was\n   ${actual}\n', info);
   }
 
-  public static function arraysNotEqual<T>(expected:Array<T>, actual:Array<T>, ?info:PosInfos):Void
+  public static function arraysNotEqual<T>(expected:Array<T>, ?actual:Array<T>, ?info:PosInfos):Void
   {
+    if (actual == null) Assert.fail('Value [$actual] is null, and cannot be compared to [$expected]', info);
     if (!expected.equals(actual)) Assert.assertionCount++;
     else
       Assert.fail('\nValue\n   ${actual}\nwas equal to\n   ${expected}\n', info);
   }
 
-  public static function pointsEqual(expected:FlxPoint, actual:FlxPoint, ?msg:String, ?info:PosInfos)
+  public static function pointsEqual(expected:FlxPoint, ?actual:FlxPoint, ?msg:String, ?info:PosInfos)
   {
+    if (actual == null) Assert.fail('Value [$actual] is null, and cannot be compared to [$expected]', info);
     if (expected.equals(actual)) Assert.assertionCount++;
     else if (msg != null) Assert.fail(msg, info);
     else
       Assert.fail("Value [" + actual + "] was not equal to expected value [" + expected + "]", info);
   }
 
-  public static function pointsNotEqual(expected:FlxPoint, actual:FlxPoint, ?msg:String, ?info:PosInfos)
+  public static function pointsNotEqual(expected:FlxPoint, ?actual:FlxPoint, ?msg:String, ?info:PosInfos)
   {
+    if (actual == null) Assert.fail('Value [$actual] is null, and cannot be compared to [$expected]', info);
     if (!expected.equals(actual)) Assert.assertionCount++;
     else if (msg != null) Assert.fail(msg, info);
     else
       Assert.fail("Value [" + actual + "] was equal to value [" + expected + "]", info);
   }
+
+  /**
+   * Execute `targetFunc`, expecting it to throw an exception.
+   * If it doesn't, or if the exception doesn't validate against the provided `predicate`, fail.
+   */
+  public static function validateThrows(targetFunc:Void->Void, predicate:Dynamic->Bool, ?info:PosInfos)
+  {
+    try
+    {
+      targetFunc();
+      Assert.fail("Expected exception to be thrown, got no failure.", info);
+    }
+    catch (e:Dynamic)
+    {
+      if (predicate(e))
+      {
+        Assert.assertionCount++;
+      }
+      else
+      {
+        Assert.fail('Expected exception to match predicate, but failed (got ${e})', info);
+      }
+    }
+  }
+
+  /**
+   * Execute `targetFunc`, expecting it to throw a `json2object.Error.CustomFunctionException` with a message matching `expected`.
+   * I made this its own function since it's the most common specific use case of `validateThrows`.
+   */
+  public static function validateThrowsJ2OCustom(targetFunc:Void->Void, expected:String, ?info:PosInfos)
+  {
+    var predicate:Dynamic->Bool = function(err:Dynamic):Bool {
+      if (!Std.isOfType(err, json2object.Error)) Assert.fail('Expected error of type json2object.Error, got ${Type.typeof(err)}');
+
+      switch (err)
+      {
+        case json2object.Error.CustomFunctionException(msg, pos):
+          if (msg != expected) Assert.fail('Expected message [${expected}], got [${msg}].');
+        default:
+          Assert.fail('Expected error of type CustomFunctionException, got [${err}].');
+      }
+
+      return true;
+    };
+    validateThrows(targetFunc, predicate, info);
+  }
 }

tests/unit/source/FunkinTest.hx

@@ -11,6 +11,7 @@ import massive.munit.Assert;
 /**
  * @see https://github.com/HaxeFlixel/flixel/tree/dev/tests/unit
  */
+@:nullSafety
 class FunkinTest
 {
   public static final MS_PER_STEP:Float = 1.0 / 60.0 * 1000;
@@ -19,7 +20,7 @@ class FunkinTest
   static inline var TICKS_PER_FRAME:UInt = 25;
   static var totalSteps:UInt = 0;
 
-  var destroyable:IFlxDestroyable;
+  var destroyable:Null<IFlxDestroyable> = null;
 
   public function new() {}
 

tests/unit/source/MockTest.hx

@@ -5,6 +5,7 @@ import massive.munit.Assert;
 import massive.munit.async.AsyncFactory;
 import funkin.util.DateUtil;
 
+@:nullSafety
 class MockTest extends FunkinTest
 {
   public function new()
@@ -45,16 +46,12 @@ class MockTest extends FunkinTest
     // If not, a VerificationException will be thrown and the test will fail.
     mockatoo.Mockatoo.verify(mockAnim.addByPrefix("testAnim", "blablabla", 24, false, false, false), times(1));
 
-    try
-    {
+    FunkinAssert.validateThrows(function() {
       // Attempt to verify the method was called.
       // This should FAIL, since we didn't call the method.
-      mockatoo.Mockatoo.verify(mockAnim.addByIndices("testAnim", "blablabla", [], "", 24, false, false, false), times(1));
-      Assert.fail("Mocking function should have thrown but didn't.");
-    }
-    catch (_:mockatoo.exception.VerificationException)
-    {
-      // Expected.
-    }
+      mockatoo.Mockatoo.verify(mockAnim.addByPrefix("testAnim", "blablabla", 24, false, false, false), times(1));
+    }, function(err) {
+      return Std.isOfType(err, mockatoo.exception.VerificationException);
+    });
   }
 }