From feb71f1d714887715fd9319c91edfe71eddb4a56 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= <sybren@blender.org>
Date: Tue, 10 Nov 2020 15:33:14 +0100
Subject: Animation: Expand unit tests for `BKE_fcurve_active_keyframe_index()`

Expand unit test for `BKE_fcurve_active_keyframe_index()` to test edge
cases better.

This also introduces a new test macro `EXPECT_BLI_ASSERT()`, which can be
used to test that an assertion fails successfully.

No functional changes to actual Blender code.
---
 .../tests/guardedalloc_overflow_test.cc            |  5 -----
 source/blender/blenkernel/intern/fcurve.c          |  2 +-
 source/blender/blenkernel/intern/fcurve_test.cc    | 26 ++++++++++++++++++++--
 tests/gtests/testing/testing.h                     | 18 +++++++++++++++
 4 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/intern/guardedalloc/tests/guardedalloc_overflow_test.cc b/intern/guardedalloc/tests/guardedalloc_overflow_test.cc
index e5754bc95ea..efbfc171fff 100644
--- a/intern/guardedalloc/tests/guardedalloc_overflow_test.cc
+++ b/intern/guardedalloc/tests/guardedalloc_overflow_test.cc
@@ -5,11 +5,6 @@
 #include "MEM_guardedalloc.h"
 
 /* We expect to abort on integer overflow, to prevent possible exploits. */
-#ifdef _WIN32
-#  define ABORT_PREDICATE ::testing::ExitedWithCode(3)
-#else
-#  define ABORT_PREDICATE ::testing::KilledBySignal(SIGABRT)
-#endif
 
 #if defined(__GNUC__) && !defined(__clang__)
 /* Disable since it's the purpose of this test. */
diff --git a/source/blender/blenkernel/intern/fcurve.c b/source/blender/blenkernel/intern/fcurve.c
index 8bfc626379f..e6d3696b198 100644
--- a/source/blender/blenkernel/intern/fcurve.c
+++ b/source/blender/blenkernel/intern/fcurve.c
@@ -855,7 +855,7 @@ void BKE_fcurve_active_keyframe_set(FCurve *fcu, const BezTriple *active_bezt)
   }
 
   /* The active keyframe should always be selected. */
-  BLI_assert(BEZT_ISSEL_ANY(active_bezt));
+  BLI_assert(BEZT_ISSEL_ANY(active_bezt) || !"active keyframe must be selected");
 
   fcu->active_keyframe_index = (int)offset;
 }
diff --git a/source/blender/blenkernel/intern/fcurve_test.cc b/source/blender/blenkernel/intern/fcurve_test.cc
index 97dd541e2b9..fb6ce02d146 100644
--- a/source/blender/blenkernel/intern/fcurve_test.cc
+++ b/source/blender/blenkernel/intern/fcurve_test.cc
@@ -22,6 +22,7 @@
 #include "BKE_fcurve.h"
 
 #include "ED_keyframing.h"
+#include "ED_types.h" /* For SELECT. */
 
 #include "DNA_anim_types.h"
 
@@ -295,17 +296,38 @@ TEST(fcurve_active_keyframe, ActiveKeyframe)
   EXPECT_EQ(BKE_fcurve_active_keyframe_index(fcu), FCURVE_ACTIVE_KEYFRAME_NONE);
 
   /* Check a "normal" action. */
+  fcu->bezt[2].f2 |= SELECT;
   BKE_fcurve_active_keyframe_set(fcu, &fcu->bezt[2]);
   EXPECT_EQ(BKE_fcurve_active_keyframe_index(fcu), 2);
 
-  /* Check out of bounds. */
+  /* Check setting an unselected keyframe as active. */
+  fcu->bezt[2].f1 = fcu->bezt[2].f2 = fcu->bezt[2].f3 = 0;
+  EXPECT_BLI_ASSERT(BKE_fcurve_active_keyframe_set(fcu, &fcu->bezt[2]),
+                    "active keyframe must be selected");
+  EXPECT_EQ(BKE_fcurve_active_keyframe_index(fcu), FCURVE_ACTIVE_KEYFRAME_NONE);
+
+  /* Check out of bounds (lower). */
   BKE_fcurve_active_keyframe_set(fcu, fcu->bezt - 20);
+  EXPECT_EQ(fcu->active_keyframe_index, FCURVE_ACTIVE_KEYFRAME_NONE)
+      << "Setting out-of-bounds value via the API should result in valid active_keyframe_index";
   EXPECT_EQ(BKE_fcurve_active_keyframe_index(fcu), FCURVE_ACTIVE_KEYFRAME_NONE);
 
-  /* Check out of bounds again. */
+  fcu->active_keyframe_index = -20;
+  EXPECT_EQ(BKE_fcurve_active_keyframe_index(fcu), FCURVE_ACTIVE_KEYFRAME_NONE)
+      << "Even with active_keyframe_index out of bounds, getting it via the API should produce a "
+         "valid value";
+
+  /* Check out of bounds (higher). */
   BKE_fcurve_active_keyframe_set(fcu, fcu->bezt + 4);
+  EXPECT_EQ(fcu->active_keyframe_index, FCURVE_ACTIVE_KEYFRAME_NONE)
+      << "Setting out-of-bounds value via the API should result in valid active_keyframe_index";
   EXPECT_EQ(BKE_fcurve_active_keyframe_index(fcu), FCURVE_ACTIVE_KEYFRAME_NONE);
 
+  fcu->active_keyframe_index = fcu->totvert;
+  EXPECT_EQ(BKE_fcurve_active_keyframe_index(fcu), FCURVE_ACTIVE_KEYFRAME_NONE)
+      << "Even with active_keyframe_index out of bounds, getting it via the API should produce a "
+         "valid value";
+
   BKE_fcurve_free(fcu);
 }
 
diff --git a/tests/gtests/testing/testing.h b/tests/gtests/testing/testing.h
index 34928035b7d..8136a93314e 100644
--- a/tests/gtests/testing/testing.h
+++ b/tests/gtests/testing/testing.h
@@ -137,4 +137,22 @@ inline void EXPECT_EQ_ARRAY_ND(const T *expected, const T *actual, const size_t
   }
 }
 
+#ifdef _WIN32
+#  define ABORT_PREDICATE ::testing::ExitedWithCode(3)
+#else
+#  define ABORT_PREDICATE ::testing::KilledBySignal(SIGABRT)
+#endif
+
+/* Test macro for when BLI_assert() is expected to fail.
+ * Note that the EXPECT_BLI_ASSERT macro is a no-op, unless used in a debug build with
+ * WITH_ASSERT_ABORT=ON. */
+#if defined(WITH_ASSERT_ABORT) && !defined(NDEBUG)
+/* EXPECT_EXIT() is used as that's the only exit-expecting function in GTest that allows us to
+ * check for SIGABRT. */
+#  define EXPECT_BLI_ASSERT(function_call, expect_message) \
+    EXPECT_EXIT(function_call, ABORT_PREDICATE, expect_message)
+#else
+#  define EXPECT_BLI_ASSERT(function_call, expect_message) function_call
+#endif
+
 #endif  // __BLENDER_TESTING_H__
-- 
cgit v1.2.3