Additional C test infrastructure #3011

paulidale · 2017-03-22T04:32:37Z

tests are added or updated

Very preliminary test infrastructure additions.

Rather than charging full steam ahead, I'd prefer some feedback to see if this is along desirable lines...

The intention is to provide a suite of functions along the lines of the existing strings_equal routine that test a condition and print a uniformly formatted message if it is false.

paulidale · 2017-03-22T04:33:19Z

@ekasper, is this along the lines of what you envisioned?

paulidale · 2017-03-22T06:49:34Z

Pretty much ignore the changes to ssl_test.c and ssl_test_ctx_test.c, these are fixing calls to strings_equal() so they compile.

The new functionality is in testutil.{c,h}.

I've updated my recent stack_test.c to use the new calls and to define a local suite of comparison functions.

levitte

Other than this, I like it so far.

levitte · 2017-03-22T06:57:35Z

test/stack_test.c

@@ -90,82 +92,67 @@ static int test_int_stack(void)

    /* Check push and num */
    for (i = 0; i < n; i++) {
-        if (sk_sint_num(s) != i) {
+        if (!int_eq(sk_sint_num(s), i, "int stack size %d", i)) {
            fprintf(stderr, "test int stack size %d\n", i);


Shouldn't this fprintf go away?

Oops, I thought I'd replaced this already :( It will go away. @jon-oracle found it soon after I made the initial commit.

levitte · 2017-03-22T07:04:23Z

test/testutil.c

 static const char *print_string_maybe_null(const char *s)
 {
    return s == NULL ? "(NULL)" : s;
 }

-int strings_equal(const char *desc, const char *s1, const char *s2)
+int strings_eq(const char *s1, const char *s2, const char *desc, ...)


A small nit; considering all other comparisons have singular names so far, it seems odd to have plural "strings" here...

Yeah, I'd picked up on this already. Should we use the singular or plural for the function names? They are an argument to the macros so either is possible.

The original function was strings_equal() but that seemed unnatural to me.

I prefer singular... not sure how to rationalise it, though. Maybe by saying that the name denotes the type, not the objects?

Okay, I'll change them to singular tomorrow.
I prefer this too :)

levitte · 2017-03-22T07:04:44Z

test/testutil.c

+    return 1;
+}
+
+int strings_ne(const char *s1, const char *s2, const char *desc, ...)


... and here

levitte · 2017-03-22T08:18:24Z

test/stack_test.c

@@ -45,6 +45,8 @@ DEFINE_SPECIAL_STACK_OF_CONST(uchar, unsigned char)
 DEFINE_STACK_OF(SS)
 DEFINE_STACK_OF_CONST(SU)

+IMPLEMENT_COMPARISONS(char, char, "%c")


I'd like to see a static variant of IMPLEMENT_COMPARISONS... it would be fitting here.

Why do we need this here rather than just adding to the common header?

I guess the question is do we want all of these test functions defined in testutil.c and used everywhere or do we want them in testutil.h so tests can create individual instances of them?

At the moment there is the flexibility to do either (although local versions aren't static). What is ideal here?

I think all the basic types should go to testutil.h (as they did). Can you give an example where tests would then still need individual instances? Are you thinking typedef'ed types?

ekasper

Yes, that's exactly what I had in mind. Thanks so much for taking this on!

ekasper · 2017-03-22T09:51:31Z

test/stack_test.c

@@ -45,6 +45,8 @@ DEFINE_SPECIAL_STACK_OF_CONST(uchar, unsigned char)
 DEFINE_STACK_OF(SS)
 DEFINE_STACK_OF_CONST(SU)

+IMPLEMENT_COMPARISONS(char, char, "%c")


Why do we need this here rather than just adding to the common header?

ekasper · 2017-03-22T09:59:21Z

test/testutil.h

 */
-int strings_equal(const char *desc, const char *s1, const char *s2);
+int bool_true(int, const char *desc, ...);


maybe test_true and test_false?

On naming, how about calling them all with a TEST_ prefix to make it stand out that these are test macros:

TEST_int_eq
TEST_true
TEST_false
TEST_ptr_not_null

And on pointers specifically, I'd probs call this TEST_ptr - less typing, and I think if (!TEST_ptr(p)) is easier to read than the double negative if (!TEST_ptr_not_null(p)).

We may also want an aborting flavour of this one. It's bad value for effort to write all the extra goto end boilerplate to exit cleanly on say malloc failures in tests (which just never happen). You don't have to do it in this PR, though.

@ekasper, how would you rename ptr_null?

Does this need its own macro? You could simply use TEST_ptr_eq(p, NULL).

Ah, so you were thinking TEST_ptr in addition to TEST_ptr_eq and TEST_ptr_ne? That escaped me, and in that case I have no concerns in the matter...

I've got some embryonic ideas as to how to do automated malloc testing -- i.e. run through all the calls to malloc and cause each to fail in turn. Having test cases that abort early due to this would be detrimental to the effort.

Are we agreed to prefix these functions with TEST_ and to shorten the ptr not null name?

ekasper · 2017-03-22T10:00:02Z

test/testutil.h

+
+/*
+ * Check a boolean result for being true or false.
+ * They return 1 if the condition is true.


"... if the condition is true (i.e., the value is non-zero)".

Within OpenSSL, 0 sometimes means true :(

ekasper · 2017-03-22T10:05:12Z

test/testutil.h

+ * the data type that was tested, |fmt| is a system provided printf format with
+ * arguments that spells out the failure details.
+ */
+#if defined(__GNUC__)


The fail_message method signature is hard to comprehend. Can you give an example in the comments how it is supposed to be used, and what the resulting output looks like?

If this is not meant to be used by tests (but only by the other macros), then perhaps move into a separate header?

I'm assuming that the output of fail_message is in a dreadful format and will be changed. I'll add better descriptions as to what its arguments are and do.

ekasper · 2017-03-22T10:07:26Z

test/testutil.c

@@ -145,19 +145,110 @@ int run_tests(const char *test_prog_name)
    return EXIT_SUCCESS;
 }

+void fail_message(const char *desc, va_list args, const char *type,


Ideally I'd like to see __FILE__ and __LINE__ here. You'd need to move the implementation to a header, though.

With __FILE__ and __LINE__ we probably won't even need desc in most cases, so we could make custom descriptions optional (less typing):

ret = TEST_int_eq(a, b);

And optionally

ret = TEST_int_eq_msg(a[i], b[i], "%d th element mismatch", i);

I don't see how adding FILE & LINE to the arguments is going to be possible. We'd want macros to do this which then call the underlying functions with the extra arguments. How do I generate the macros automatically? Generating the functions is easy enough and doing macros for predefined ones would be possible (by listing them out). How would I go with e.g. IMPLEMENT_COMPARISONS(struct fred *, fredptrs, "%p")
I'd need to generate fredpts_eq(...) and the wrapper macro TEST_fredpts_eq(...).

Is there even an OpenSSL approved method of varadic macros?

Not as far as I know. That came with later C standards...

It would be possible to have macro versions of these without the description or at least without the description with arguments. That might be sufficient. I'll mulled it over a bit....

ekasper · 2017-03-22T10:11:08Z

test/testutil.h

+    DEFINE_COMPARISONS(type, name, fmt)
+
+/*
+ * Define eight comparisons between pairs of integers and unsigned integers.


It'd be nice if you could spell out, at least once here, what the resulting method signatures are.

ekasper · 2017-03-22T10:19:16Z

test/ssl_test.c

-    ret &= strings_equal("ExpectedNPNProtocol",
-                         test_ctx->expected_npn_protocol,
-                         result->client_npn_negotiated);
+    ret &= strings_eq(result->client_npn_negotiated,


Here's an idea for a future PR: collect all errors in a static variable and make each test automatically fail if one of the test assertions fails.

Not sure how ugly the implementation would be with the tooling we have (C90) but worth a thought.

Collecting the errors wouldn't be too hard. A stack of strings, malloc buffers and sprintf into them.
Fail if the stack isn't empty.

paulidale · 2017-03-22T22:54:24Z

~~Currently not done: FILE & LINE support via macros~~.
~~Prefixing functions with TEST_. I used test_ instead~~.
I think I've addressed the other concerns.

paulidale · 2017-03-23T00:37:17Z

How's this? I've added TEST_ macros that use the description field to display the file and line number -- the user doesn't get to provide a description here but they can always fall back to the underlying function directly for this.

I've updated some of the tests in stack_test.c to use the macros.

richsalz · 2017-03-23T01:40:45Z

we have the tooling to do malloc failures; look at the code in crypto/mem.c

richsalz · 2017-03-23T01:44:41Z

No varadic macro support.

paulidale · 2017-03-23T01:47:35Z

I've seen the crypto mem code, it is sufficient. I'm still figuring out a good way to attack the problem so such testing can become integral to the test framework. There's little point doing ad-hoc tests, each implemented differently. It's a WIP but I'm reasonably confident it's workable. But that is for later.

ekasper

Cool!

ekasper · 2017-03-23T13:18:04Z

test/testutil.h

+ * The following macros provide wrapper calls to the test functions with
+ * a default description that indicates the file and line number of the error.
+ */
+#define TEST_int_eq(a, b)   test_int_eq(a, b, "%s: %d", __FILE__, __LINE__)


This looks good! However I'd still like to preserve file and line numbers for the custom error messages too. Now we can't do that in general (because no support for variadic macros in C90) but you could do something like this I think?

# if !defined(__STDC_VERSION__) || __STDC_VERSION__ < 199901L # define TEST_error test_error_c90 #else # define TEST_error(...) test_error(__FILE__, __LINE__, __VA_ARGS__) #endif

where test_error_c90 takes a format string and args, and test_error additionally takes the file and line number.

Then hide the lowercase functions in a separate internal header (testutil_internal.h) so that tests don't use them directly. Code that previously called test_int_eq would do

if (!TEST_int_eq(a[i], b[i])) { TEST_error("%d th element mismatch", i); }

Or you could also provide a macro per comparison, TEST_int_eq_msg(a, b, msg), etc.

What do you think?

I'm not convinced that TEST_error needs to show the file and line number. With the usage example given, the file and line will be produced by TEST_int_eq and would be redundant. Perhaps call the function TEST_extra or TEST_info

I don't like the TEST_int_eq_msg suggestion because we don't have variadic macros so there can be no arguments to the message.

Are you suggesting hiding the test_xxx functions in an header but still creating them in testutil.c ?

I meant you'd do TEST_int_eq_msgsame as TEST_error - variadic macros where you can, and fall back to a function (losing the file and line number) otherwise. I think that works?

Re file and line numbers, I don't think they ever hurt. I'd probably have TEST_error for errors (starts with ERROR or FAIL as you have it) and TEST_info for info (starts with INFO) to make it easy to grep for failure lines - but I don't really have a strong opinion. Whatever feels most natural to you!

Re test_xxx functions, I think so? It's not so important - I only suggested moving the declarations to another header to make it easier to find the public bits you're supposed to use in tests.

I think the TEST_xxx_msg flavours can be made to work, it will introduce quite a bit of bloat to the header file -- is the gain worthwhile? The if (TEST_xxx()) TEST_ERROR() sequence seems reasonable and sufficiently terse. At least to me. I kind of suspect that the TEST_xxx__msg versions won't be heavily used -- people often seem to not like writing tests and when they do, they make them as short and unhelpful as possible. Slightly exaggerated perhaps.

You're right, the line numbers won't hurt. I'll add the info versions.

The declarations need to be visible in testutil.h to avoid compiler warnings about undefined functions. Whether they are in another .h file or not seems to be the concern. I'd don't have a strong feeling either way here.

ekasper · 2017-03-23T13:18:52Z

test/testutil.h

+#define TEST_uchar_gt(a, b) test_uchar_gt(a, b, "%s: %d", __FILE__, __LINE__)
+#define TEST_uchar_ge(a, b) test_uchar_ge(a, b, "%s: %d", __FILE__, __LINE__)
+
+#define TEST_lint_eq(a, b)  test_lint_eq(a, b, "%s: %d", __FILE__, __LINE__)


I'd suggest long and ulong ("lint" makes me think of linters).

ekasper · 2017-03-23T13:27:25Z

test/testutil.c

+DEFINE_COMPARISONS(unsigned int, uint, "%u")
+DEFINE_COMPARISONS(char, char, "%c")
+DEFINE_COMPARISONS(unsigned char, uchar, "%u")
+DEFINE_COMPARISONS(long int, lint, "%ld")


Simply long and unsigned long (drop the int).

paulidale · 2017-03-23T23:14:02Z

I think this addresses the next round of changes. I've added the TEST_error macro and hidden the definition macros (in testutil.c rather than a separate header file).

paulidale · 2017-03-24T00:29:22Z

Regarding the TEST_xxx_msg flavours, I can't help but feel that they aren't worthwhile.
The whole point of these tests is that they should pass and the message is never seen. If a test does fail, somebody is going to have to go and debug the code and the file + line number is adequate to get started. A message is a nicety but isn't really necessary. Plus messages are available with test_ERROR and test_INFO and can easily be added to a failing test.

ekasper

Nice call on testing the tests!

This looks good to me, a few small comments left. I'll let the second reviewer take a fresh look, then let's see it in action.

You may want to post to openssl-dev@ pointing out this work, so devs know to start using this over printf.

ekasper · 2017-03-28T09:49:20Z

test/testutil.c

+            PRINTF_FORMAT(5, 6);
+
+static void test_fail_message_va(int error, const char *file, int line,
+                              const char *type, const char *fmt, va_list ap)


nit: bad indent

I'd just pass in const char* prefix rather than int error, makes the call site more readable.

Good suggestion :)

ekasper · 2017-03-28T10:01:30Z

test/testutil.c

+
+    for (i=0; i<2*n; i++) {
+        unsigned char c = (i & 1) != 0 ? p[i / 2] & 15 : p[i / 2] >> 4;
+        out[i] = c["0123456789abcdef"];


Is this c90?

That's C since way back. A little quirk of the language. Ever tried this?

#include <stdio.h> int main() { printf("%c", 3["123456789"]); }

Of course, the other way around, "123456789"[3], is just as possible and equivalent.

yeah, let's flip it around to avoid questions.

This originally comes from K&R C and has remained ever since. a[b] is defined to be *(a + b). Thus, the order doesn't matter.

Little quirk in my coding to hex conversions...

ekasper · 2017-03-28T10:03:23Z

test/cipherlist_test.c

-            fprintf(stderr, "Wrong cipher at position %d: expected %x, "
-                    "got %x\n", i, expected_cipher_id, cipher_id);
+        if (!TEST_int_eq(cipher_id, expected_cipher_id)) {
+            TEST_info("Wrong cipher at position %d: expected %x, "


TEST_error? (Everywhere where errors are logged?)

The TEST_int_eq will produce the error message. This is extra additional info: the position where the error occurred. I don't think it should raise a second error, since it isn't. I'll get rid of the duplicated information though. There are probably a few cases like this still.

ekasper · 2017-03-28T10:10:58Z

test/test_test.c

+ */
+
+/*
+ * Copyright (c) 2017 Oracle and/or its affiliates.  All rights reserved.


I believe we avoid custom copyright statements in file headers these days. (The CLA does not transfer copyright - just that additional copyrights don't need to be listed in our headers.)

paulidale · 2017-03-28T21:39:07Z

Should be good to go. Run make test TESTS=test_test V=1 to see the output of failed messages. All the information is there but if anyone feels changes are desirable, we should do them before any announcement.

I've also updated test/README to not include the bit about editing the makefile and to have a short section about this.

paulidale · 2017-03-28T21:44:48Z

Any thoughts about dedicating a code health Tuesday to modifying existing test cases to use this infrastructure more uniformly?

richsalz · 2017-03-28T22:12:17Z

@pauli-oracle that's a great idea! I'll push it. Looks like @levitte;s conerns were address. I also approve and can merge.

paulidale · 2017-03-28T22:14:46Z

Thanks @richsalz. The single build failure is due to a problem with documentation (i.e. not part of this PR).

richsalz · 2017-03-28T22:32:13Z

I needed these changes:

diff --git a/test/test_test.c b/test/test_test.c
index 842b449..de39917 100644
--- a/test/test_test.c
+++ b/test/test_test.c
@@ -254,8 +254,9 @@ err:

 static int test_messages(void)
 {
-    TEST_INFO("This is an %s message.", "info");
-    TEST_ERROR("This is an %s message.", "error");
+    TEST_info("This is an %s message.", "info");
+    TEST_error("This is an %s message.", "error");
+    return 0;
 }

And also the test_test recipe needs to be changed. Not sure how to say "failure is success" @levitte ?

paulidale · 2017-03-28T22:40:52Z

Sorry, I hadn't pushed that capitalisation change yet. I realised I'd not executed those two functions in the test. Done now.

There is no need to modify the recipe, the test_test.c program checks the failing return codes and acts accordingly. The failure messages will be printed out but the executable returns the correct code and the test passes.

paulidale · 2017-03-28T22:42:03Z

This could be a pain when grepping for ERRORs.

I also just noticed that there is no test name in the diagnostics...

richsalz · 2017-03-28T22:51:44Z

TEST_INFO is a struct, as you saw I assume.Do you want to push a change to fix the logging issue?

richsalz · 2017-03-28T22:57:38Z

@levitte or @ekasper reconfirm?

paulidale · 2017-03-28T23:00:13Z

I've fixed the test_test.c issue. I'm not concerned about the test name -- we've got file/line number information in the output already so it isn't essential. The error messages are a little busy already.

richsalz · 2017-03-28T23:02:28Z

So we just need a second reviewer for that last bit.

paulidale · 2017-03-28T23:25:46Z

Both lines of it :)

t-j-h · 2017-03-28T23:31:36Z

Which two lines?

paulidale · 2017-03-28T23:33:33Z

test/test_test.c lines 263 & 264. Well, actually that whole six line function and the line at the bottom that adds it as a test. But only two lines of content :)

paulidale · 2017-03-28T23:35:28Z

static int test_messages(void)
{
    TEST_info("This is an %s message.", "info");
    TEST_error("This is an %s message.", "error");
    return 1;
}

and

    ADD_TEST(test_messages);

t-j-h · 2017-03-28T23:38:23Z

+1 on the "last bit".

Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Emilia Käsper <emilia@openssl.org> Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3011)

richsalz · 2017-03-29T03:40:28Z

merged. thanks for this, and the reviews folks!

ekasper · 2017-03-29T12:06:25Z

+1 on a Code Health Tuesday, it'd give everyone a chance to learn the framework. Also, we have a bunch of really old tests (for example, ectest.c) that could really use some clean-up.

Thanks again for this contribution, really neat!

paulidale · 2017-03-29T21:08:21Z

The other code health thought I've had is "fix an example day". I.e. go through the examples and get them working and bring them up to date or write new examples.

paulidale force-pushed the test-infra branch from 2195949 to e9db9bc Compare March 22, 2017 04:36

levitte requested changes Mar 22, 2017

View reviewed changes

ekasper suggested changes Mar 22, 2017

View reviewed changes

ekasper added the code-health label Mar 22, 2017

paulidale force-pushed the test-infra branch 2 times, most recently from b3f34af to 5f4105f Compare March 22, 2017 22:28

paulidale force-pushed the test-infra branch 3 times, most recently from 16b7603 to d9c6f75 Compare March 23, 2017 00:35

paulidale force-pushed the test-infra branch from d9c6f75 to d866499 Compare March 23, 2017 00:49

paulidale force-pushed the test-infra branch 2 times, most recently from d82e65e to 61dcece Compare March 23, 2017 02:40

ekasper suggested changes Mar 23, 2017

View reviewed changes

paulidale force-pushed the test-infra branch from 61dcece to 90483e2 Compare March 23, 2017 23:11

paulidale force-pushed the test-infra branch from 90483e2 to 1e0d653 Compare March 23, 2017 23:55

paulidale force-pushed the test-infra branch 5 times, most recently from 6b7f617 to c548cc1 Compare March 24, 2017 03:50

ekasper approved these changes Mar 28, 2017

View reviewed changes

paulidale force-pushed the test-infra branch 2 times, most recently from acbe207 to 1460a7f Compare March 28, 2017 21:37

paulidale force-pushed the test-infra branch from 1460a7f to 5a73c01 Compare March 28, 2017 22:11

richsalz self-assigned this Mar 28, 2017

richsalz approved these changes Mar 28, 2017

View reviewed changes

Test infrastructure additions.

74251f6

paulidale force-pushed the test-infra branch from 32deccf to 74251f6 Compare March 28, 2017 22:51

richsalz closed this Mar 29, 2017

paulidale deleted the test-infra branch March 29, 2017 22:42

Additional C test infrastructure #3011

Additional C test infrastructure #3011

Conversation

paulidale commented Mar 22, 2017

paulidale commented Mar 22, 2017

paulidale commented Mar 22, 2017

levitte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulidale Mar 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekasper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulidale commented Mar 22, 2017 • edited

paulidale commented Mar 23, 2017

richsalz commented Mar 23, 2017

richsalz commented Mar 23, 2017 via email

paulidale commented Mar 23, 2017

ekasper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulidale commented Mar 23, 2017

paulidale commented Mar 24, 2017

ekasper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulidale commented Mar 28, 2017

paulidale commented Mar 28, 2017

richsalz commented Mar 28, 2017

paulidale commented Mar 28, 2017

richsalz commented Mar 28, 2017

paulidale commented Mar 28, 2017

paulidale commented Mar 28, 2017

richsalz commented Mar 28, 2017

richsalz commented Mar 28, 2017

paulidale commented Mar 28, 2017

richsalz commented Mar 28, 2017 via email

paulidale commented Mar 28, 2017

t-j-h commented Mar 28, 2017

paulidale commented Mar 28, 2017 • edited

paulidale Mar 22, 2017 •

edited

paulidale commented Mar 22, 2017 •

edited

paulidale commented Mar 28, 2017 •

edited