diff mbox series

[for_v2?,v2,08/14] selftests/harness: Move operator macros to their own header file

Message ID 20191017030340.18301-9-sean.j.christopherson@intel.com (mailing list archive)
State New, archived
Headers show
Series selftests/x86/sgx: Improve tests | expand

Commit Message

Sean Christopherson Oct. 17, 2019, 3:03 a.m. UTC 
Move the operator macros, ASSERT_* and EXTEND_*, to a standalone header
so that they can be reused by other selftests without pulling in the
full harness framework, which is cumbersome to use for testing features
that require a substantial amount of setup, need callbacks, etc...

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 Documentation/dev-tools/kselftest.rst         |   9 +-
 MAINTAINERS                                   |   1 +
 tools/testing/selftests/kselftest_harness.h   | 246 +----------------
 tools/testing/selftests/kselftest_operators.h | 255 ++++++++++++++++++
 4 files changed, 259 insertions(+), 252 deletions(-)
 create mode 100644 tools/testing/selftests/kselftest_operators.h

Comments

Jarkko Sakkinen Oct. 17, 2019, 4:53 p.m. UTC | #1 
On Wed, Oct 16, 2019 at 08:03:34PM -0700, Sean Christopherson wrote:
> Move the operator macros, ASSERT_* and EXTEND_*, to a standalone header
> so that they can be reused by other selftests without pulling in the
> full harness framework, which is cumbersome to use for testing features
> that require a substantial amount of setup, need callbacks, etc...
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Is it possible to just use a "dull" selftest and not go into this before
the code is upstreamed? If yes, lets go with that. Do not mind using
frameworky stuff later on. Just trying to avoid new intersects with
other subsystems, that's all.

/Jarkko
Sean Christopherson Oct. 17, 2019, 6:13 p.m. UTC | #2 
On Thu, Oct 17, 2019 at 07:53:56PM +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 16, 2019 at 08:03:34PM -0700, Sean Christopherson wrote:
> > Move the operator macros, ASSERT_* and EXTEND_*, to a standalone header
> > so that they can be reused by other selftests without pulling in the
> > full harness framework, which is cumbersome to use for testing features
> > that require a substantial amount of setup, need callbacks, etc...
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> Is it possible to just use a "dull" selftest and not go into this before
> the code is upstreamed? If yes, lets go with that.

It's certainly possible, but the code is verbose and ugly (IMO), which
means it will be harder for other to review.

> Do not mind using frameworky stuff later on. Just trying to avoid new
> intersects with other subsystems, that's all.

Understood, but IMO this particular change is safe-ish:

  - We're already intersecting with selftests
  - The odds of a conflict are very low
  - Andy is already involved in SGX


There are also other options:

  1. Use kselftest_harness.h as-is via #undef __EXPECT.  I actually would
     have gone with this option except the header also defines a function
     and we end up with a compiler error due to an unused function :-(

  2. Upstream the change separately from SGX, similar to the
     FEATURE_CONTROL handling.  E.g. I think the KVM selftests could
     use the operators, so there would even be concrete usage.
Jarkko Sakkinen Oct. 21, 2019, 11:08 a.m. UTC | #3 
On Thu, Oct 17, 2019 at 11:13:09AM -0700, Sean Christopherson wrote:
> On Thu, Oct 17, 2019 at 07:53:56PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Oct 16, 2019 at 08:03:34PM -0700, Sean Christopherson wrote:
> > > Move the operator macros, ASSERT_* and EXTEND_*, to a standalone header
> > > so that they can be reused by other selftests without pulling in the
> > > full harness framework, which is cumbersome to use for testing features
> > > that require a substantial amount of setup, need callbacks, etc...
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > Is it possible to just use a "dull" selftest and not go into this before
> > the code is upstreamed? If yes, lets go with that.
> 
> It's certainly possible, but the code is verbose and ugly (IMO), which
> means it will be harder for other to review.

Ok, I'll try to explain in more verbose terms how I see this.

Not all selftests use the harness and I'm not yet confident that SGX has
to. Unfortunately, ugly is for me something that I cannot put metrics
on. Also, often "ugly" is actually better than layering because it is
more transparent.

The test is comprised of simple POSIX calls that everyone knows whereas
using kselftest harness requires learning new framework. Less macros
makes code also easier to debug and pair compare to dissembly when
required. I've done the latter at least a few times.

It will also add a requirement for code reviewers who are simply looking
for a code example how SGX works also to learn the harness. In the scope
of the patch set the selftest serves as a such example.

/Jarkko
Sean Christopherson Oct. 22, 2019, 3:20 a.m. UTC | #4 
On Mon, Oct 21, 2019 at 02:08:23PM +0300, Jarkko Sakkinen wrote:
> On Thu, Oct 17, 2019 at 11:13:09AM -0700, Sean Christopherson wrote:
> > On Thu, Oct 17, 2019 at 07:53:56PM +0300, Jarkko Sakkinen wrote:
> > > On Wed, Oct 16, 2019 at 08:03:34PM -0700, Sean Christopherson wrote:
> > > > Move the operator macros, ASSERT_* and EXTEND_*, to a standalone header
> > > > so that they can be reused by other selftests without pulling in the
> > > > full harness framework, which is cumbersome to use for testing features
> > > > that require a substantial amount of setup, need callbacks, etc...
> > > > 
> > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > 
> > > Is it possible to just use a "dull" selftest and not go into this before
> > > the code is upstreamed? If yes, lets go with that.
> > 
> > It's certainly possible, but the code is verbose and ugly (IMO), which
> > means it will be harder for other to review.
> 
> Ok, I'll try to explain in more verbose terms how I see this.
> 
> Not all selftests use the harness and I'm not yet confident that SGX has
> to. Unfortunately, ugly is for me something that I cannot put metrics
> on. Also, often "ugly" is actually better than layering because it is
> more transparent.
> 
> The test is comprised of simple POSIX calls that everyone knows whereas
> using kselftest harness requires learning new framework. Less macros
> makes code also easier to debug and pair compare to dissembly when
> required. I've done the latter at least a few times.
> 
> It will also add a requirement for code reviewers who are simply looking
> for a code example how SGX works also to learn the harness. In the scope
> of the patch set the selftest serves as a such example.

Eh, if SGX were actually using any of the harness stuff, sure, but I'd
hope most reviewers intuitively understand what ASSERT_EQ does.
diff mbox series

Patch

diff --git a/Documentation/dev-tools/kselftest.rst b/Documentation/dev-tools/kselftest.rst
index 25604904fa6e..09dbeb8ab502 100644
--- a/Documentation/dev-tools/kselftest.rst
+++ b/Documentation/dev-tools/kselftest.rst
@@ -290,12 +290,5 @@  Helpers
 Operators
 ---------
 
-.. kernel-doc:: tools/testing/selftests/kselftest_harness.h
-    :doc: operators
+.. kernel-doc:: tools/testing/selftests/kselftest_operators.h
 
-.. kernel-doc:: tools/testing/selftests/kselftest_harness.h
-    :functions: ASSERT_EQ ASSERT_NE ASSERT_LT ASSERT_LE ASSERT_GT ASSERT_GE
-                ASSERT_NULL ASSERT_TRUE ASSERT_NULL ASSERT_TRUE ASSERT_FALSE
-                ASSERT_STREQ ASSERT_STRNE EXPECT_EQ EXPECT_NE EXPECT_LT
-                EXPECT_LE EXPECT_GT EXPECT_GE EXPECT_NULL EXPECT_TRUE
-                EXPECT_FALSE EXPECT_STREQ EXPECT_STRNE
diff --git a/MAINTAINERS b/MAINTAINERS
index 1eb065d3c209..71d680dff071 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14514,6 +14514,7 @@  F:	include/uapi/linux/seccomp.h
 F:	include/linux/seccomp.h
 F:	tools/testing/selftests/seccomp/*
 F:	tools/testing/selftests/kselftest_harness.h
+F:	tools/testing/selftests/kselftest_operators.h
 F:	Documentation/userspace-api/seccomp_filter.rst
 K:	\bsecure_computing
 K:	\bTIF_SECCOMP\b
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 5336b26506ab..89af5a7bfd65 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -62,6 +62,8 @@ 
 #include <sys/wait.h>
 #include <unistd.h>
 
+#include "kselftest_operators.h"
+
 #define TEST_TIMEOUT_DEFAULT 30
 
 /* Utilities exposed to the test definitions */
@@ -343,250 +345,6 @@ 
 		return test_harness_run(argc, argv); \
 	}
 
-/**
- * DOC: operators
- *
- * Operators for use in TEST() and TEST_F().
- * ASSERT_* calls will stop test execution immediately.
- * EXPECT_* calls will emit a failure warning, note it, and continue.
- */
-
-/**
- * ASSERT_EQ(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * ASSERT_EQ(expected, measured): expected == measured
- */
-#define ASSERT_EQ(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, ==, 1)
-
-/**
- * ASSERT_NE(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * ASSERT_NE(expected, measured): expected != measured
- */
-#define ASSERT_NE(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, !=, 1)
-
-/**
- * ASSERT_LT(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * ASSERT_LT(expected, measured): expected < measured
- */
-#define ASSERT_LT(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, <, 1)
-
-/**
- * ASSERT_LE(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * ASSERT_LE(expected, measured): expected <= measured
- */
-#define ASSERT_LE(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, <=, 1)
-
-/**
- * ASSERT_GT(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * ASSERT_GT(expected, measured): expected > measured
- */
-#define ASSERT_GT(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, >, 1)
-
-/**
- * ASSERT_GE(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * ASSERT_GE(expected, measured): expected >= measured
- */
-#define ASSERT_GE(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, >=, 1)
-
-/**
- * ASSERT_NULL(seen)
- *
- * @seen: measured value
- *
- * ASSERT_NULL(measured): NULL == measured
- */
-#define ASSERT_NULL(seen) \
-	__EXPECT(NULL, "NULL", seen, #seen, ==, 1)
-
-/**
- * ASSERT_TRUE(seen)
- *
- * @seen: measured value
- *
- * ASSERT_TRUE(measured): measured != 0
- */
-#define ASSERT_TRUE(seen) \
-	__EXPECT(0, "0", seen, #seen, !=, 1)
-
-/**
- * ASSERT_FALSE(seen)
- *
- * @seen: measured value
- *
- * ASSERT_FALSE(measured): measured == 0
- */
-#define ASSERT_FALSE(seen) \
-	__EXPECT(0, "0", seen, #seen, ==, 1)
-
-/**
- * ASSERT_STREQ(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * ASSERT_STREQ(expected, measured): !strcmp(expected, measured)
- */
-#define ASSERT_STREQ(expected, seen) \
-	__EXPECT_STR(expected, seen, ==, 1)
-
-/**
- * ASSERT_STRNE(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * ASSERT_STRNE(expected, measured): strcmp(expected, measured)
- */
-#define ASSERT_STRNE(expected, seen) \
-	__EXPECT_STR(expected, seen, !=, 1)
-
-/**
- * EXPECT_EQ(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * EXPECT_EQ(expected, measured): expected == measured
- */
-#define EXPECT_EQ(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, ==, 0)
-
-/**
- * EXPECT_NE(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * EXPECT_NE(expected, measured): expected != measured
- */
-#define EXPECT_NE(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, !=, 0)
-
-/**
- * EXPECT_LT(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * EXPECT_LT(expected, measured): expected < measured
- */
-#define EXPECT_LT(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, <, 0)
-
-/**
- * EXPECT_LE(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * EXPECT_LE(expected, measured): expected <= measured
- */
-#define EXPECT_LE(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, <=, 0)
-
-/**
- * EXPECT_GT(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * EXPECT_GT(expected, measured): expected > measured
- */
-#define EXPECT_GT(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, >, 0)
-
-/**
- * EXPECT_GE(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * EXPECT_GE(expected, measured): expected >= measured
- */
-#define EXPECT_GE(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, >=, 0)
-
-/**
- * EXPECT_NULL(seen)
- *
- * @seen: measured value
- *
- * EXPECT_NULL(measured): NULL == measured
- */
-#define EXPECT_NULL(seen) \
-	__EXPECT(NULL, "NULL", seen, #seen, ==, 0)
-
-/**
- * EXPECT_TRUE(seen)
- *
- * @seen: measured value
- *
- * EXPECT_TRUE(measured): 0 != measured
- */
-#define EXPECT_TRUE(seen) \
-	__EXPECT(0, "0", seen, #seen, !=, 0)
-
-/**
- * EXPECT_FALSE(seen)
- *
- * @seen: measured value
- *
- * EXPECT_FALSE(measured): 0 == measured
- */
-#define EXPECT_FALSE(seen) \
-	__EXPECT(0, "0", seen, #seen, ==, 0)
-
-/**
- * EXPECT_STREQ(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * EXPECT_STREQ(expected, measured): !strcmp(expected, measured)
- */
-#define EXPECT_STREQ(expected, seen) \
-	__EXPECT_STR(expected, seen, ==, 0)
-
-/**
- * EXPECT_STRNE(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * EXPECT_STRNE(expected, measured): strcmp(expected, measured)
- */
-#define EXPECT_STRNE(expected, seen) \
-	__EXPECT_STR(expected, seen, !=, 0)
-
 #define ARRAY_SIZE(a)	(sizeof(a) / sizeof(a[0]))
 
 /* Support an optional handler after and ASSERT_* or EXPECT_*.  The approach is
diff --git a/tools/testing/selftests/kselftest_operators.h b/tools/testing/selftests/kselftest_operators.h
new file mode 100644
index 000000000000..6ae5b547313f
--- /dev/null
+++ b/tools/testing/selftests/kselftest_operators.h
@@ -0,0 +1,255 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * See documentation in Documentation/dev-tools/kselftest.rst
+ */
+
+#ifndef __KSELFTEST_OPERATORS_H
+#define __KSELFTEST_OPERATORS_H
+
+/**
+ * DOC:
+ *
+ * Operators for use in Test Harness's TEST() and TEST_F(), or with a custom
+ * implementation of __EXPECT().
+ * ASSERT_* calls will stop test execution immediately.
+ * EXPECT_* calls will emit a failure warning, note it, and continue.
+ */
+
+/**
+ * ASSERT_EQ(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * ASSERT_EQ(expected, measured): expected == measured
+ */
+#define ASSERT_EQ(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, ==, 1)
+
+/**
+ * ASSERT_NE(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * ASSERT_NE(expected, measured): expected != measured
+ */
+#define ASSERT_NE(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, !=, 1)
+
+/**
+ * ASSERT_LT(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * ASSERT_LT(expected, measured): expected < measured
+ */
+#define ASSERT_LT(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, <, 1)
+
+/**
+ * ASSERT_LE(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * ASSERT_LE(expected, measured): expected <= measured
+ */
+#define ASSERT_LE(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, <=, 1)
+
+/**
+ * ASSERT_GT(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * ASSERT_GT(expected, measured): expected > measured
+ */
+#define ASSERT_GT(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, >, 1)
+
+/**
+ * ASSERT_GE(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * ASSERT_GE(expected, measured): expected >= measured
+ */
+#define ASSERT_GE(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, >=, 1)
+
+/**
+ * ASSERT_NULL(seen)
+ *
+ * @seen: measured value
+ *
+ * ASSERT_NULL(measured): NULL == measured
+ */
+#define ASSERT_NULL(seen) \
+	__EXPECT(NULL, "NULL", seen, #seen, ==, 1)
+
+/**
+ * ASSERT_TRUE(seen)
+ *
+ * @seen: measured value
+ *
+ * ASSERT_TRUE(measured): measured != 0
+ */
+#define ASSERT_TRUE(seen) \
+	__EXPECT(0, "0", seen, #seen, !=, 1)
+
+/**
+ * ASSERT_FALSE(seen)
+ *
+ * @seen: measured value
+ *
+ * ASSERT_FALSE(measured): measured == 0
+ */
+#define ASSERT_FALSE(seen) \
+	__EXPECT(0, "0", seen, #seen, ==, 1)
+
+/**
+ * ASSERT_STREQ(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * ASSERT_STREQ(expected, measured): !strcmp(expected, measured)
+ */
+#define ASSERT_STREQ(expected, seen) \
+	__EXPECT_STR(expected, seen, ==, 1)
+
+/**
+ * ASSERT_STRNE(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * ASSERT_STRNE(expected, measured): strcmp(expected, measured)
+ */
+#define ASSERT_STRNE(expected, seen) \
+	__EXPECT_STR(expected, seen, !=, 1)
+
+/**
+ * EXPECT_EQ(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * EXPECT_EQ(expected, measured): expected == measured
+ */
+#define EXPECT_EQ(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, ==, 0)
+
+/**
+ * EXPECT_NE(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * EXPECT_NE(expected, measured): expected != measured
+ */
+#define EXPECT_NE(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, !=, 0)
+
+/**
+ * EXPECT_LT(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * EXPECT_LT(expected, measured): expected < measured
+ */
+#define EXPECT_LT(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, <, 0)
+
+/**
+ * EXPECT_LE(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * EXPECT_LE(expected, measured): expected <= measured
+ */
+#define EXPECT_LE(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, <=, 0)
+
+/**
+ * EXPECT_GT(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * EXPECT_GT(expected, measured): expected > measured
+ */
+#define EXPECT_GT(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, >, 0)
+
+/**
+ * EXPECT_GE(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * EXPECT_GE(expected, measured): expected >= measured
+ */
+#define EXPECT_GE(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, >=, 0)
+
+/**
+ * EXPECT_NULL(seen)
+ *
+ * @seen: measured value
+ *
+ * EXPECT_NULL(measured): NULL == measured
+ */
+#define EXPECT_NULL(seen) \
+	__EXPECT(NULL, "NULL", seen, #seen, ==, 0)
+
+/**
+ * EXPECT_TRUE(seen)
+ *
+ * @seen: measured value
+ *
+ * EXPECT_TRUE(measured): 0 != measured
+ */
+#define EXPECT_TRUE(seen) \
+	__EXPECT(0, "0", seen, #seen, !=, 0)
+
+/**
+ * EXPECT_FALSE(seen)
+ *
+ * @seen: measured value
+ *
+ * EXPECT_FALSE(measured): 0 == measured
+ */
+#define EXPECT_FALSE(seen) \
+	__EXPECT(0, "0", seen, #seen, ==, 0)
+
+/**
+ * EXPECT_STREQ(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * EXPECT_STREQ(expected, measured): !strcmp(expected, measured)
+ */
+#define EXPECT_STREQ(expected, seen) \
+	__EXPECT_STR(expected, seen, ==, 0)
+
+/**
+ * EXPECT_STRNE(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * EXPECT_STRNE(expected, measured): strcmp(expected, measured)
+ */
+#define EXPECT_STRNE(expected, seen) \
+	__EXPECT_STR(expected, seen, !=, 0)
+
+#endif  /* __KSELFTEST_OPERATORS_H */
+