[libc][test] adds errno clearer test fixture, gtest-style errno and fp except assertions #91608
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-libc Author: Michael Flanders (Flandini) ChangesAdds errno, fp exception matchers similar to gtest's The goal is to start moving fp tests from just checking equality to also These new matchers wrap statements, block statements, or expressions like so: or I think this design will make it easier to convert existing tests as progress is I was thinking in #88816 that we should switch to RAII style matchers, but I My worry with these changes is the large amount of FP-related assertions there I've also converted a few existing test cases to use these new matchers as Patch is 101.13 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/91608.diff 158 Files Affected:
diff --git a/libc/test/UnitTest/CMakeLists.txt b/libc/test/UnitTest/CMakeLists.txt
index 302af3044ca3d..bc7fb88c8ff36 100644
--- a/libc/test/UnitTest/CMakeLists.txt
+++ b/libc/test/UnitTest/CMakeLists.txt
@@ -108,6 +108,40 @@ add_header_library(
libc.src.__support.CPP.type_traits
)
+add_unittest_framework_library(
+ LibcFPExceptionHelpers
+ SRCS
+ FPExceptMatcher.cpp
+ HDRS
+ FPExceptMatcher.h
+ DEPENDS
+ LibcTest
+ libc.src.__support.FPUtil.fp_bits
+ libc.src.__support.FPUtil.fenv_impl
+ libc.hdr.types.fenv_t
+ libc.hdr.fenv_macros
+)
+
+add_header_library(
+ ErrnoSetterMatcher
+ HDRS
+ ErrnoSetterMatcher.h
+ DEPENDS
+ libc.src.__support.common
+ libc.src.__support.FPUtil.fp_bits
+ libc.src.__support.StringUtil.error_to_string
+ libc.src.errno.errno
+)
+
+add_header_library(
+ ErrnoSafeTest
+ HDRS
+ ErrnoSafeTest.h
+ DEPENDS
+ libc.src.__support.CPP.utility
+ libc.src.errno.errno
+)
+
add_unittest_framework_library(
LibcFPTestHelpers
SRCS
@@ -116,30 +150,22 @@ add_unittest_framework_library(
HDRS
FEnvSafeTest.h
FPMatcher.h
+ FPTest.h
RoundingModeUtils.h
DEPENDS
LibcTest
+ ErrnoSafeTest
+ ErrnoSetterMatcher
+ LibcFPExceptionHelpers
libc.test.UnitTest.string_utils
libc.src.__support.CPP.array
+ libc.src.__support.CPP.utility
libc.src.__support.FPUtil.fp_bits
libc.src.__support.FPUtil.fpbits_str
libc.src.__support.FPUtil.fenv_impl
libc.src.__support.FPUtil.rounding_mode
)
-add_unittest_framework_library(
- LibcFPExceptionHelpers
- SRCS
- FPExceptMatcher.cpp
- HDRS
- FPExceptMatcher.h
- DEPENDS
- LibcTest
- libc.src.__support.FPUtil.fp_bits
- libc.src.__support.FPUtil.fenv_impl
- libc.hdr.types.fenv_t
-)
-
add_unittest_framework_library(
LibcMemoryHelpers
SRCS
@@ -176,14 +202,3 @@ add_unittest_framework_library(
libc.src.stdio.scanf_core.core_structs
libc.test.UnitTest.string_utils
)
-
-add_header_library(
- ErrnoSetterMatcher
- HDRS
- ErrnoSetterMatcher.h
- DEPENDS
- libc.src.__support.common
- libc.src.__support.FPUtil.fp_bits
- libc.src.__support.StringUtil.error_to_string
- libc.src.errno.errno
-)
diff --git a/libc/test/UnitTest/ErrnoSafeTest.h b/libc/test/UnitTest/ErrnoSafeTest.h
new file mode 100644
index 0000000000000..bad4ab764ac0b
--- /dev/null
+++ b/libc/test/UnitTest/ErrnoSafeTest.h
@@ -0,0 +1,26 @@
+//===-- ErrnoSafeTestFixture.h ---------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===---------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_TEST_UNITTEST_ERRNOSAFETEST_H
+#define LLVM_LIBC_TEST_UNITTEST_ERRNOSAFETEST_H
+
+#include "src/__support/CPP/utility.h"
+#include "src/errno/libc_errno.h"
+#include "test/UnitTest/Test.h"
+
+namespace LIBC_NAMESPACE::testing {
+
+// This is a test fixture for clearing errno before the start of a test case.
+class ErrnoSafeTest : virtual public Test {
+public:
+ void SetUp() override { LIBC_NAMESPACE::libc_errno = 0; }
+};
+
+} // namespace LIBC_NAMESPACE::testing
+
+#endif // LLVM_LIBC_TEST_UNITTEST_ERRNOSAFETEST_H
diff --git a/libc/test/UnitTest/ErrnoSetterMatcher.h b/libc/test/UnitTest/ErrnoSetterMatcher.h
index 745ba4182023e..d0f284b733309 100644
--- a/libc/test/UnitTest/ErrnoSetterMatcher.h
+++ b/libc/test/UnitTest/ErrnoSetterMatcher.h
@@ -58,6 +58,9 @@ template <typename T> struct Comparator {
#endif
};
+// TODO: this should check errno and not always need a return value to
+// also compare against. The FP and non-FP matching is redundant with
+// the other matchers pulled in through Test.h and FPMatcher.h.
template <typename T> class ErrnoSetterMatcher : public Matcher<T> {
Comparator<T> return_cmp;
Comparator<int> errno_cmp;
@@ -184,4 +187,37 @@ static ErrnoSetterMatcherBuilder<RetT> returns(internal::Comparator<RetT> cmp) {
} // namespace testing
} // namespace LIBC_NAMESPACE
+// Used to check that `LIBC_NAMESPACE::libc_errno` was 0 or a specific
+// errno after executing `expr_or_statement` from a state where
+// `LIBC_NAMESPACE::libc_errno` was 0. This is generic, so does not check
+// `math_errhandling & MATH_ERRNO` before errno matching, see FPTest.h for
+// assertions that check this.
+//
+// Expects `expected` to be convertible to int type.
+//
+// Does not return the value of expr_or_statement, i.e., intended usage
+// is: `EXPECT_ERRNO(EDOM, EXPECT_EQ(..., ...));` or
+// ```
+// EXPECT_ERRNO(EDOM, {
+// stmt;
+// ...
+// });
+// ```
+//
+// TODO: this currently uses `ErrnoSetterMatcher` for the nice explanation on
+// failed errno matching. `ErrnoSetterMatcher` requires a return value to also
+// always check, so this code always checks 0 against 0 for the return value--
+// it is not actually checking the value of `expr_or_statement` per above doc
+// comments. When `ErrnoSetterMatcher` is changed to not always check return
+// values, change this also.
+#define EXPECT_ERRNO(expected, expr_or_statement) \
+ do { \
+ LIBC_NAMESPACE::libc_errno = 0; \
+ expr_or_statement; \
+ EXPECT_THAT( \
+ 0, LIBC_NAMESPACE::testing::internal::ErrnoSetterMatcher<int>( \
+ LIBC_NAMESPACE::testing::ErrnoSetterMatcher::EQ(0), \
+ LIBC_NAMESPACE::testing::ErrnoSetterMatcher::EQ((expected)))); \
+ } while (0)
+
#endif // LLVM_LIBC_TEST_ERRNOSETTERMATCHER_H
diff --git a/libc/test/UnitTest/FEnvSafeTest.h b/libc/test/UnitTest/FEnvSafeTest.h
index d5a8bb7ee667c..6ac9fcf89ffd2 100644
--- a/libc/test/UnitTest/FEnvSafeTest.h
+++ b/libc/test/UnitTest/FEnvSafeTest.h
@@ -18,7 +18,7 @@ namespace LIBC_NAMESPACE::testing {
// This provides a test fixture (or base class for other test fixtures) that
// asserts that each test does not leave the FPU state represented by `fenv_t`
// (aka `FPState`) perturbed from its initial state.
-class FEnvSafeTest : public Test {
+class FEnvSafeTest : virtual public Test {
public:
void TearDown() override;
diff --git a/libc/test/UnitTest/FPExceptMatcher.cpp b/libc/test/UnitTest/FPExceptMatcher.cpp
index c1dfc53924662..c10da56e07d92 100644
--- a/libc/test/UnitTest/FPExceptMatcher.cpp
+++ b/libc/test/UnitTest/FPExceptMatcher.cpp
@@ -10,6 +10,7 @@
#include "test/UnitTest/Test.h"
+#include "hdr/fenv_macros.h"
#include "hdr/types/fenv_t.h"
#include "src/__support/FPUtil/FEnvImpl.h"
#include <memory>
@@ -35,7 +36,7 @@ static void sigfpeHandler(int sig) {
siglongjmp(jumpBuffer, -1);
}
-FPExceptMatcher::FPExceptMatcher(FunctionCaller *func) {
+FPExceptCallableMatcher::FPExceptCallableMatcher(FunctionCaller *func) {
auto oldSIGFPEHandler = signal(SIGFPE, &sigfpeHandler);
std::unique_ptr<FunctionCaller> funcUP(func);
diff --git a/libc/test/UnitTest/FPExceptMatcher.h b/libc/test/UnitTest/FPExceptMatcher.h
index 5136e381081ee..12adaeb259c5a 100644
--- a/libc/test/UnitTest/FPExceptMatcher.h
+++ b/libc/test/UnitTest/FPExceptMatcher.h
@@ -9,6 +9,7 @@
#ifndef LLVM_LIBC_TEST_UNITTEST_FPEXCEPTMATCHER_H
#define LLVM_LIBC_TEST_UNITTEST_FPEXCEPTMATCHER_H
+#include "hdr/fenv_macros.h"
#include "test/UnitTest/Test.h"
#include "test/UnitTest/TestLogger.h"
@@ -17,9 +18,71 @@
namespace LIBC_NAMESPACE {
namespace testing {
+// Used to compare FP exception flag states with nice error printing
+class FPExceptMatcher : public Matcher<int> {
+ const int expected;
+ int actual;
+
+public:
+ explicit FPExceptMatcher(int expected) : expected(expected) {}
+
+ void explainError() override {
+ tlog << "Expected floating point exceptions: " << expected << ' ';
+ printExcepts(expected);
+ tlog << '\n';
+
+ tlog << "Actual floating point exceptions: " << actual << ' ';
+ printExcepts(actual);
+ tlog << '\n';
+ }
+
+ bool match(int got) {
+ actual = got;
+ return got == expected;
+ }
+
+private:
+ void printExcepts(int excepts) {
+ if (!excepts) {
+ tlog << "(no exceptions)";
+ return;
+ }
+
+ bool firstPrinted = false;
+ auto printWithPipe = [&](const char *name) {
+ if (firstPrinted)
+ tlog << "| ";
+
+ tlog << name;
+
+ firstPrinted = true;
+ };
+
+ tlog << '(';
+
+ if (FE_DIVBYZERO & excepts)
+ printWithPipe("FE_DIVBYZERO");
+
+ if (FE_INEXACT & excepts)
+ printWithPipe("FE_INEXACT");
+
+ if (FE_INVALID & excepts)
+ printWithPipe("FE_INVALID");
+
+ if (FE_OVERFLOW & excepts)
+ printWithPipe("FE_OVERFLOW");
+
+ if (FE_UNDERFLOW & excepts)
+ printWithPipe("FE_UNDERFLOW");
+
+ tlog << ')';
+ }
+};
+
// TODO: Make the matcher match specific exceptions instead of just identifying
// that an exception was raised.
-class FPExceptMatcher : public Matcher<bool> {
+// Used in death tests for fenv
+class FPExceptCallableMatcher : public Matcher<bool> {
bool exceptionRaised;
public:
@@ -40,7 +103,7 @@ class FPExceptMatcher : public Matcher<bool> {
}
// Takes ownership of func.
- explicit FPExceptMatcher(FunctionCaller *func);
+ explicit FPExceptCallableMatcher(FunctionCaller *func);
bool match(bool unused) { return exceptionRaised; }
@@ -53,11 +116,42 @@ class FPExceptMatcher : public Matcher<bool> {
} // namespace testing
} // namespace LIBC_NAMESPACE
+// Matches on the FP exception flag `expected` being *equal* to FP exception
+// flag `actual`
+#define EXPECT_FP_EXCEPT_EQUAL(expected, actual) \
+ EXPECT_THAT((actual), LIBC_NAMESPACE::testing::FPExceptMatcher((expected)))
+
+#define ASSERT_FP_EXCEPT_EQUAL(expected, actual) \
+ ASSERT_THAT((actual), LIBC_NAMESPACE::testing::FPExceptMatcher((expected)))
+
#define ASSERT_RAISES_FP_EXCEPT(func) \
ASSERT_THAT( \
true, \
- LIBC_NAMESPACE::testing::FPExceptMatcher( \
- LIBC_NAMESPACE::testing::FPExceptMatcher::getFunctionCaller(func)))
+ LIBC_NAMESPACE::testing::FPExceptCallableMatcher( \
+ LIBC_NAMESPACE::testing::FPExceptCallableMatcher::getFunctionCaller( \
+ func)))
+
+// Does not return the value of `expr_or_statement`, i.e., intended usage
+// is: `EXPECT_FP_EXCEPT(FE_INVALID, EXPECT_FP_EQ(..., ...));` or
+// ```
+// EXPECT_FP_EXCEPT(FE_ALL_EXCEPT, {
+// stmt;
+// ...
+// });
+// ```
+// Ensures that fp excepts are cleared before executing `expr_or_statement`
+// Checking (expected = 0) should ensure that no exceptions were set
+#define EXPECT_FP_EXCEPT(expected, expr_or_statement) \
+ do { \
+ LIBC_NAMESPACE::fputil::clear_except(FE_ALL_EXCEPT); \
+ expr_or_statement; \
+ int expected_ = (expected); \
+ int mask_ = expected_ ? expected_ : FE_ALL_EXCEPT; \
+ if (math_errhandling & MATH_ERREXCEPT) { \
+ EXPECT_FP_EXCEPT_EQUAL(expected_, \
+ LIBC_NAMESPACE::fputil::test_except(mask_)); \
+ } \
+ } while (0)
#else // !LIBC_TEST_HAS_MATCHERS()
diff --git a/libc/test/UnitTest/FPMatcher.h b/libc/test/UnitTest/FPMatcher.h
index 26af5cec02b58..8a803a8c4ddcf 100644
--- a/libc/test/UnitTest/FPMatcher.h
+++ b/libc/test/UnitTest/FPMatcher.h
@@ -61,60 +61,9 @@ template <TestCond C, typename T> FPMatcher<T, C> getMatcher(T expectedValue) {
return FPMatcher<T, C>(expectedValue);
}
-template <typename T> struct FPTest : public Test {
- using FPBits = LIBC_NAMESPACE::fputil::FPBits<T>;
- using StorageType = typename FPBits::StorageType;
- static constexpr StorageType STORAGE_MAX =
- LIBC_NAMESPACE::cpp::numeric_limits<StorageType>::max();
- static constexpr T zero = FPBits::zero(Sign::POS).get_val();
- static constexpr T neg_zero = FPBits::zero(Sign::NEG).get_val();
- static constexpr T aNaN = FPBits::quiet_nan().get_val();
- static constexpr T sNaN = FPBits::signaling_nan().get_val();
- static constexpr T inf = FPBits::inf(Sign::POS).get_val();
- static constexpr T neg_inf = FPBits::inf(Sign::NEG).get_val();
- static constexpr T min_normal = FPBits::min_normal().get_val();
- static constexpr T max_normal = FPBits::max_normal().get_val();
- static constexpr T min_denormal = FPBits::min_subnormal().get_val();
- static constexpr T max_denormal = FPBits::max_subnormal().get_val();
-
- static constexpr int N_ROUNDING_MODES = 4;
- static constexpr fputil::testing::RoundingMode ROUNDING_MODES[4] = {
- fputil::testing::RoundingMode::Nearest,
- fputil::testing::RoundingMode::Upward,
- fputil::testing::RoundingMode::Downward,
- fputil::testing::RoundingMode::TowardZero,
- };
-};
-
} // namespace testing
} // namespace LIBC_NAMESPACE
-#define DECLARE_SPECIAL_CONSTANTS(T) \
- using FPBits = LIBC_NAMESPACE::fputil::FPBits<T>; \
- using StorageType = typename FPBits::StorageType; \
- \
- static constexpr StorageType STORAGE_MAX = \
- LIBC_NAMESPACE::cpp::numeric_limits<StorageType>::max(); \
- const T zero = FPBits::zero(Sign::POS).get_val(); \
- const T neg_zero = FPBits::zero(Sign::NEG).get_val(); \
- const T aNaN = FPBits::quiet_nan().get_val(); \
- const T sNaN = FPBits::signaling_nan().get_val(); \
- const T inf = FPBits::inf(Sign::POS).get_val(); \
- const T neg_inf = FPBits::inf(Sign::NEG).get_val(); \
- const T min_normal = FPBits::min_normal().get_val(); \
- const T max_normal = FPBits::max_normal(Sign::POS).get_val(); \
- const T neg_max_normal = FPBits::max_normal(Sign::NEG).get_val(); \
- const T min_denormal = FPBits::min_subnormal(Sign::POS).get_val(); \
- const T neg_min_denormal = FPBits::min_subnormal(Sign::NEG).get_val(); \
- const T max_denormal = FPBits::max_subnormal().get_val(); \
- static constexpr int UNKNOWN_MATH_ROUNDING_DIRECTION = 99; \
- static constexpr LIBC_NAMESPACE::cpp::array<int, 6> \
- MATH_ROUNDING_DIRECTIONS_INCLUDING_UNKNOWN = { \
- FP_INT_UPWARD, FP_INT_DOWNWARD, \
- FP_INT_TOWARDZERO, FP_INT_TONEARESTFROMZERO, \
- FP_INT_TONEAREST, UNKNOWN_MATH_ROUNDING_DIRECTION, \
- };
-
#define EXPECT_FP_EQ(expected, actual) \
EXPECT_THAT(actual, LIBC_NAMESPACE::testing::getMatcher< \
LIBC_NAMESPACE::testing::TestCond::EQ>(expected))
@@ -188,36 +137,30 @@ template <typename T> struct FPTest : public Test {
EXPECT_FP_EXCEPTION(expected_except); \
} while (0)
-#define EXPECT_FP_EQ_ALL_ROUNDING(expected, actual) \
+#define FOR_ROUNDING_(rounding_mode, expr_or_statement) \
do { \
using namespace LIBC_NAMESPACE::fputil::testing; \
- ForceRoundingMode __r1(RoundingMode::Nearest); \
- if (__r1.success) { \
- EXPECT_FP_EQ((expected), (actual)); \
- } \
- ForceRoundingMode __r2(RoundingMode::Upward); \
- if (__r2.success) { \
- EXPECT_FP_EQ((expected), (actual)); \
- } \
- ForceRoundingMode __r3(RoundingMode::Downward); \
- if (__r3.success) { \
- EXPECT_FP_EQ((expected), (actual)); \
- } \
- ForceRoundingMode __r4(RoundingMode::TowardZero); \
- if (__r4.success) { \
- EXPECT_FP_EQ((expected), (actual)); \
+ ForceRoundingMode __r((rounding_mode)); \
+ if (__r.success) { \
+ expr_or_statement; \
} \
} while (0)
-#define EXPECT_FP_EQ_ROUNDING_MODE(expected, actual, rounding_mode) \
+#define FOR_ALL_ROUNDING_(expr_or_statement) \
do { \
using namespace LIBC_NAMESPACE::fputil::testing; \
- ForceRoundingMode __r((rounding_mode)); \
- if (__r.success) { \
- EXPECT_FP_EQ((expected), (actual)); \
- } \
+ FOR_ROUNDING_(RoundingMode::Nearest, expr_or_statement); \
+ FOR_ROUNDING_(RoundingMode::Upward, expr_or_statement); \
+ FOR_ROUNDING_(RoundingMode::Downward, expr_or_statement); \
+ FOR_ROUNDING_(RoundingMode::TowardZero, expr_or_statement); \
} while (0)
+#define EXPECT_FP_EQ_ALL_ROUNDING(expected, actual) \
+ FOR_ALL_ROUNDING_(EXPECT_FP_EQ((expected), (actual)))
+
+#define EXPECT_FP_EQ_ROUNDING_MODE(expected, actual, rounding_mode) \
+ FOR_ROUNDING_(rounding_mode, EXPECT_FP_EQ((expected), (actual)))
+
#define EXPECT_FP_EQ_ROUNDING_NEAREST(expected, actual) \
EXPECT_FP_EQ_ROUNDING_MODE((expected), (actual), RoundingMode::Nearest)
diff --git a/libc/test/UnitTest/FPTest.h b/libc/test/UnitTest/FPTest.h
new file mode 100644
index 0000000000000..0d530cdff9c39
--- /dev/null
+++ b/libc/test/UnitTest/FPTest.h
@@ -0,0 +1,112 @@
+//===-- FPTest.h -----------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===---------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_TEST_UNITTEST_FPTEST_H
+#define LLVM_LIBC_TEST_UNITTEST_FPTEST_H
+
+#include "src/__support/CPP/utility.h"
+#include "src/__support/FPUtil/FEnvImpl.h"
+#include "src/__support/FPUtil/FPBits.h"
+#include "test/UnitTest/ErrnoSafeTest.h" // Test fixture for clearing errno
+#include "test/UnitTest/ErrnoSetterMatcher.h" // Per-assertion clear/check errno
+#include "test/UnitTest/FEnvSafeTest.h" // Test fixture for resetting fenv
+#include "test/UnitTest/FPExceptMatcher.h" // Per-assertion clear/c...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Adding @lntue for review. |
|
re: gtest api from the May 9 libc meeting: I can change these matchers and the matcher/matcherbase classes to match the gtest api. Do we want the expect/assert macros to also be a subset of gtest's, i.e., not have these special FP errno/except assert/expects? If the functions and their tests are being individually picked and ran with gtest rather than LibcTest, then macros seem less of a problem (besides not being usable outside of test class members) and more the matcher/test/fixture class apis, right? cc @frobtech |
lntue
requested review from
gchatelet,
SchrodingerZhu,
lntue,
frobtech and
michaelrj-google
| RoundingModeUtils.h | ||
| DEPENDS | ||
| LibcTest | ||
| ErrnoSafeTest |
There was a problem hiding this comment.
We usually add .ErrnoSetterMatcher as relative paths. Is it better to unify the style?
cc @lntue
There was a problem hiding this comment.
I'm down to make the change. Should we change the other names in this file too? lmk
There was a problem hiding this comment.
Ah, sorry for missing the reply. I am fine with unifying all of them at once.
| @@ -18,7 +18,7 @@ namespace LIBC_NAMESPACE::testing { | |||
| // This provides a test fixture (or base class for other test fixtures) that | |||
| // asserts that each test does not leave the FPU state represented by `fenv_t` | |||
| // (aka `FPState`) perturbed from its initial state. | |||
| class FEnvSafeTest : public Test { | |||
| class FEnvSafeTest : virtual public Test { | |||
There was a problem hiding this comment.
why does this need to be virtual now?
There was a problem hiding this comment.
I changed this to virtual to let FPTest subclass both fixtures:
template <typename T> class FPTest :
public ErrnoSafeTest, public FEnvSafeTest {...
libc/test/UnitTest/FPExceptMatcher.h
Outdated
| @@ -53,11 +116,42 @@ class FPExceptMatcher : public Matcher<bool> { | |||
| } // namespace testing | |||
| } // namespace LIBC_NAMESPACE | |||
|
|
|||
| // Matches on the FP exception flag `expected` being *equal* to FP exception | |||
| // flag `actual` | |||
| #define EXPECT_FP_EXCEPT_EQUAL(expected, actual) \ | |||
There was a problem hiding this comment.
nit: I'd prefer to use EXCEPTION_EQ rather than EXCEPT_EQUAL. I find having the words EXPECT and EXCEPT in the same macro name hard to read.
There was a problem hiding this comment.
Made the changes in 56e8d88. I've also changed all the other macros with both EXPECT and EXCEPT in the identifier. After doing this, there was a name collision with EXPECT_FP_EXCEPTION(exception) we were previously using. I changed this to EXPECT_FP_EXCEPTION_HAPPENED(exception) and changed all uses in the tests, so it's a large commit.
Like the other matchers we have, the matchers added in this PR does not match the gtest API on the matchers, and the rest is macros around expect/assert macros that I believe are all compatible with the gtest API. I started working on a separate PR to change the other matchers to use the gtest API, and can circle back to these matchers or have this PR wait til the other is in, up to y'all. |
michaelrj-google
approved these changes
May 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds errno, fp exception matchers similar to gtest's
EXPECT_THROW.The goal is to start moving fp tests from just checking equality to also
always checking errno and fp exception flags. The current matchers work fine
for this goal, but I think we could benefit from standardizing what fp tests
should look like: moving test setup involving errno and fp exceptions from the
test cases to fixtures (see changes to
cosf_test), normalizing per-assertionset-up to clear state before executing code (compared to now where it is cleared
after and dependent on existing tests to always have cleared which is not always
the case), and changing rounding mode tests to behave well with assertions on
errno and fp exceptions.
These new matchers wrap statements, block statements, or expressions like so:
or
I think this design will make it easier to convert existing tests as progress is
made on #88819. Compared to a different approach or the one with the existing
assertions in FPMatcher.h, this allows grouping statements which might also
benefit other tests (fenv, errno), and doesn't restrict to also always matching
an expression's value which is hard to fit to void functions and functions like
sincosf. I think macros are also nicer than passing function or closureobjects around because it is less to write at each assertion and we get to keep
the EXPECT/ASSERT behavior difference, though these are still necessary for the
death tests that test errno, exceptions, and signals.
I was thinking in #88816 that we should switch to RAII style matchers, but I
tried that out, and the test failure locations are nicer with the clearing of
errno/fp exception flags just spelled out in the assertion macro bodies.
My worry with these changes is the large amount of FP-related assertions there
would be if this is merged. If this PR looks good to y'all, I would say that
the preferred way to write FP tests is to just include FPTest.h and use these
new assertions and work on phasing out the other errno/exception related
matchers in FPMatcher.h as #88816 is worked through.
I've also converted a few existing test cases to use these new matchers as
exemplars and for testing the changes.