Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger

[Brendan Higgins <brendanhiggins@google.com>]

On Thu, Jul 18, 2019 at 12:22:33PM -0700, Brendan Higgins wrote:
> On Thu, Jul 18, 2019 at 10:50 AM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Brendan Higgins (2019-07-16 11:52:01)
> > > On Tue, Jul 16, 2019 at 10:50 AM Stephen Boyd <sboyd@kernel.org> wrote:
[...]
> > Do you have a link to those earlier patches?
> 
> This is the first patchset:
> 
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788057.html
> 
> In particular you can see the code for matching functions here:
> 
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788073.html
> 
> And parameter matching code here:
> 
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788072.html
> 
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788086.html
> 
> My apologies in advance, but the code at this early stage had not
> adopted the kunit_* prefix and was still using the test_* and mock_*
> prefix. (Hence, struct kunit_stream was known as struct test_stream).
[...]
> > The crux of my complaint is that the string stream API is too loosely
> > defined to be usable. It allows tests to build up a string of
> > unstructured information, but with certain calling constraints so we
> > have to tread carefully. If there was more structure to the data that's
> > being recorded then the test case runner could operate on the data
> > without having to do string/stream operations, allocations, etc. This
> > would make the assertion logic much more concrete and specific to kunit,
> > instead of this small kunit wrapper that's been placed on top of string
> > stream.
> 
> Yeah, I can see the point of wanting something that provides more
> structure than the raw `struct kunit_stream` interface. In fact, it is
> something I had already started working on, when I had determined it
> would be a large effort to capture all the variations. I was further
> put off from the idea when I had been asked to convert the KUnit
> intermediate format from what I was using to TAP, because, as it is,
> the current data printed out by KUnit doesn't contain all the data I
> would like to put in it in a way that best takes advantage of the TAP
> specification. One problematic area in particular: TAP already
> provides a way to present a lot of the data I would like to export,
> but it involves JSON serialization which was an idea that some of the
> other reviewers understandably weren't too keen on. TAP also wants to
> report data some time after it is available, which is generally not a
> good idea for test debug information; you want to make it available as
> soon as you can or you risk crashing with the data still inside.
> 
> Hence, I decided we could probably spend a good long while debating
> how I present the information. So the idea of having a loose
> definition seemed attractive to me in its own right since it would
> likely conform to whatever we ended up deciding in the long run. Also,
> all the better that it was what I already had and no one seemed to
> mind too much.
> 
> The only constant I expect is that `struct kunit` will likely need to
> take an abstract object with a `commit` method, or a `format` method
> or whatever so it could control when data was going to be printed out
> to the user. We will probably also use a string builder in there
> somewhere.
> 
> > TL;DR: If we can get rid of the string stream API I'd view that as an
> > improvement because building arbitrary strings in the kernel is complex,
> > error prone and has calling context concerns.
> 
> True. No argument there.
> 
> > Is the intention that other code besides unit tests will use this string
> > stream API to build up strings? Any targets in mind? This would be a
> > good way to get the API merged upstream given that its 2019 and we
> > haven't had such an API in the kernel so far.
> 
> Someone, (was it you?) asked about code sharing with a string builder
> thingy that was used for creating structured human readable files, but
> that seemed like a pretty massive undertaking.
> 
> Aside from that, no. I would kind of prefered that nobody used it for
> anything else because I the issues you described.
> 
> Nevertheless, I think the debate over the usefulness of the
> string_stream and kunit_stream are separate topics. Even if we made
> kunit_stream more structured, I am pretty sure I would want to use
> string_stream or some variation for constructing the message.
> 
> > An "object oriented" (strong quotes!) approach where kunit_fail_msg is
> > the innermost struct in some assertion specific structure might work
> > nicely and allow the test runner to call a generic 'format' function to
> > print out the message based on the type of assertion/expectation it is.
> > It probably would mean less code size too because the strings that are
> > common will be in the common printing function instead of created twice,
> > in the macros/code and then copied to the heap for the string stream.
> >
> >         struct kunit_assert {
> >                 const char *line;
> >                 const char *file;
> >                 const char *func;
> >                 void (*format)(struct kunit_assert *assert);
> >         };
> >
> >         struct kunit_comparison_assert {
> >                 enum operator operator;
> >                 const char *left;
> >                 const char *right;
> >                 struct kunit_assert assert;
> >         };
> >
> >         struct kunit_bool_assert {
> >                 const char *truth;
> >                 const char *statement;
> >                 struct kunit_assert assert;
> >         };
> >
> >         void kunit_format_comparison(struct kunit_assert *assert)
> >         {
> >                 struct kunit_comparison_assert *comp = container_of(assert, ...)
> >
> >                 kunit_printk(...)
> >         }

I started poking around with your suggestion while we are waiting. A
couple early observations:

1) It is actually easier to do than I previously thought and will probably
   help with getting more of the planned TAP output stuff working.

   That being said, this is still a pretty substantial undertaking and
   will likely take *at least* a week to implement and properly review.
   Assuming everything goes extremely well (no unexpected issues on my
   end, very responsive reviewers, etc).

2) It *will* eliminate the need for kunit_stream.

3) ...but, it *will not* eliminate the need for string_stream.

Based on my early observations, I do think it is worth doing, but I
don't think it is worth trying to make it in this patchset (unless I
have already missed the window, or it is going to be open for a while):
I do think it will make things much cleaner, but I don't think it will
achieve your desired goal of getting rid of an unstructured
{kunit|string}_stream style interface; it just adds a layer on top of it
that makes it harder to misuse.

I attached a patch of what I have so far at the end of this email so you
can see what I am talking about. And of course, if you agree with my
assessment, so we can start working on it as a future patch.

A couple things in regard to the patch I attached:

1) I wrote it pretty quickly so there are almost definitely mistakes.
   You should consider it RFC. I did verify it compiles though.

2) Also, I did use kunit_stream in writing it: all occurences should be
   pretty easy to replace with string_stream; nevertheless, the reason
   for this is just to make it easier to play with the current APIs. I
   wanted to have something working before I went through a big tedious
   refactoring. So sorry if it causes any confusion.

3) I also based the patch on all the KUnit patches I have queued up
   (includes things like mocking and such) since I want to see how this
   serialization thing will work with mocks and matchers and things like
   that.

> I started working on something similarish, but by the time I ended up
> coming up with a parent object whose definition was loose enough to
> satisfy all the properties required by the child classes it ended up
> basically being the same as what I have now just with a more complex
> hierarchy of message manipulation logic.
> 
> On the other hand, I didn't have the idea of doing the parent object
> quite the way you did and that would clean up a lot of the duplicated
> first line logic.
> 
> I would like to give it a try, but I am afraid I am going to get
> sucked down a really deep rabbit hole.
> 
> > Maybe other people have opinions here on if you should do it now or
> > later. Future coding is not a great argument because it's hard to
> > predict the future. On the other hand, this patchset is in good shape to
> 
> Yeah, that's kind of why I am afraid to go down this road when I have
> something that works now and I know works with the mocking stuff I
> want to do.
> 
> I would like to try your suggestion, but I want to try to make it work
> with my mocking patches before I commit to it because otherwise I am
> just going to have to back it out in a follow up patchset.
> 
> > merge and I'd like to use it to write unit tests for code I maintain so
> > I don't want to see this stall out. Sorry if I'm opening the can of
> > worms you're talking about.
> 
> Don't be sorry. I agree with you that the kunit_stream stuff is not very pretty.
> 
> Shuah, have we missed the merge window for 5.3?
> 
> I saw you only sent one PR out so far for this release, and there
> wasn't much in it; I imagine you are going to send at least one more?
> 
> I figure, if we still got time to try out your suggestion, Stephen, no
> harm in trying.
> 
> Also if we missed it, then I have another couple months to play around with it.
> 
> What do you think?

I attached the patch mentioned above below. Let me know what you think!

Cheers!

From 53d475d3d56afcf92b452c6d347dbedfa1a17d34 Mon Sep 17 00:00:00 2001
From: Brendan Higgins <brendanhiggins@google.com>
Date: Thu, 18 Jul 2019 16:08:52 -0700
Subject: [PATCH v1] DO NOT MERGE: started playing around with the
 serialization api

---
 include/kunit/assert.h | 130 ++++++++++++++++++++++++++++++
 include/kunit/mock.h   |   4 +
 kunit/Makefile         |   3 +-
 kunit/assert.c         | 179 +++++++++++++++++++++++++++++++++++++++++
 kunit/mock.c           |   6 +-
 5 files changed, 318 insertions(+), 4 deletions(-)
 create mode 100644 include/kunit/assert.h
 create mode 100644 kunit/assert.c

diff --git a/include/kunit/assert.h b/include/kunit/assert.h
new file mode 100644
index 0000000000000..e054fdff4642f
--- /dev/null
+++ b/include/kunit/assert.h
@@ -0,0 +1,130 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Assertion and expectation serialization API.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#ifndef _KUNIT_ASSERT_H
+#define _KUNIT_ASSERT_H
+
+#include <kunit/test.h>
+#include <kunit/mock.h>
+
+enum kunit_assert_type {
+	KUNIT_ASSERTION,
+	KUNIT_EXPECTATION,
+};
+
+struct kunit_assert {
+	enum kunit_assert_type type;
+	const char *line;
+	const char *file;
+	struct va_format message;
+	void (*format)(struct kunit_assert *assert,
+		       struct kunit_stream *stream);
+};
+
+void kunit_base_assert_format(struct kunit_assert *assert,
+			      struct kunit_stream *stream);
+
+void kunit_assert_print_msg(struct kunit_assert *assert,
+			    struct kunit_stream *stream);
+
+struct kunit_unary_assert {
+	struct kunit_assert assert;
+	const char *condition;
+	bool expected_true;
+};
+
+void kunit_unary_assert_format(struct kunit_assert *assert,
+			       struct kunit_stream *stream);
+
+struct kunit_ptr_not_err_assert {
+	struct kunit_assert assert;
+	const char *text;
+	void *value;
+};
+
+void kunit_ptr_not_err_assert_format(struct kunit_assert *assert,
+				     struct kunit_stream *stream);
+
+struct kunit_binary_assert {
+	struct kunit_assert assert;
+	const char *operation;
+	const char *left_text;
+	long long left_value;
+	const char *right_text;
+	long long right_value;
+};
+
+void kunit_binary_assert_format(struct kunit_assert *assert,
+				struct kunit_stream *stream);
+
+struct kunit_binary_ptr_assert {
+	struct kunit_assert assert;
+	const char *operation;
+	const char *left_text;
+	void *left_value;
+	const char *right_text;
+	void *right_value;
+};
+
+void kunit_binary_ptr_assert_format(struct kunit_assert *assert,
+				    struct kunit_stream *stream);
+
+struct kunit_binary_str_assert {
+	struct kunit_assert assert;
+	const char *operation;
+	const char *left_text;
+	const char *left_value;
+	const char *right_text;
+	const char *right_value;
+};
+
+void kunit_binary_str_assert_format(struct kunit_assert *assert,
+				    struct kunit_stream *stream);
+
+struct kunit_mock_assert {
+	struct kunit_assert assert;
+};
+
+struct kunit_mock_no_expectations {
+	struct kunit_mock_assert assert;
+};
+
+struct kunit_mock_declaration {
+	const char *function_name;
+	const char **type_names;
+	const void **params;
+	int len;
+};
+
+void kunit_mock_declaration_format(struct kunit_mock_declaration *declaration,
+				   struct kunit_stream *stream);
+
+struct kunit_matcher_result {
+	struct kunit_assert assert;
+};
+
+struct kunit_mock_failed_match {
+	struct list_head node;
+	const char *expectation_text;
+	struct kunit_matcher_result *matcher_list;
+	size_t matcher_list_len;
+};
+
+void kunit_mock_failed_match_format(struct kunit_mock_failed_match *match,
+				    struct kunit_stream *stream);
+
+struct kunit_mock_no_match {
+	struct kunit_mock_assert assert;
+	struct kunit_mock_declaration declaration;
+	struct list_head failed_match_list;
+};
+
+void kunit_mock_no_match_format(struct kunit_assert *assert,
+				struct kunit_stream *stream);
+
+#endif /*  _KUNIT_ASSERT_H */
diff --git a/include/kunit/mock.h b/include/kunit/mock.h
index 001b96af62f1e..52c9e427c831b 100644
--- a/include/kunit/mock.h
+++ b/include/kunit/mock.h
@@ -144,6 +144,10 @@ void mock_register_formatter(struct mock_param_formatter *formatter);
 
 void mock_unregister_formatter(struct mock_param_formatter *formatter);
 
+void mock_format_param(struct kunit_stream *stream,
+		       const char *type_name,
+		       const void *param);
+
 struct mock *mock_get_global_mock(void);
 
 #define MOCK(name) name##_mock
diff --git a/kunit/Makefile b/kunit/Makefile
index bbf43fcfb93a9..149d856a30f04 100644
--- a/kunit/Makefile
+++ b/kunit/Makefile
@@ -3,7 +3,8 @@ obj-$(CONFIG_KUNIT) +=			test.o \
 					common-mocks.o \
 					string-stream.o \
 					kunit-stream.o \
-					try-catch.o
+					try-catch.o \
+					assert.o
 
 obj-$(CONFIG_KUNIT_TEST) +=		test-test.o \
 					test-mock.o \
diff --git a/kunit/assert.c b/kunit/assert.c
new file mode 100644
index 0000000000000..75bb6922a994e
--- /dev/null
+++ b/kunit/assert.c
@@ -0,0 +1,179 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Assertion and expectation serialization API.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+#include <kunit/assert.h>
+
+void kunit_base_assert_format(struct kunit_assert *assert,
+			      struct kunit_stream *stream)
+{
+	const char *expect_or_assert;
+
+	if (assert->type == KUNIT_EXPECTATION)
+		expect_or_assert = "EXPECTATION";
+	else
+		expect_or_assert = "ASSERTION";
+
+	kunit_stream_add(stream, "%s FAILED at %s:%s\n",
+			 expect_or_assert, assert->file, assert->line);
+}
+
+void kunit_assert_print_msg(struct kunit_assert *assert,
+			    struct kunit_stream *stream)
+{
+	if (assert->message.fmt)
+		kunit_stream_add(stream, "\n%pV", &assert->message);
+}
+
+void kunit_unary_assert_format(struct kunit_assert *assert,
+			       struct kunit_stream *stream)
+{
+	struct kunit_unary_assert *unary_assert = container_of(
+			assert, struct kunit_unary_assert, assert);
+
+	kunit_base_assert_format(assert, stream);
+	if (unary_assert->expected_true)
+		kunit_stream_add(stream,
+				 "\tExpected %s to be true, but is false\n",
+				 unary_assert->condition);
+	else
+		kunit_stream_add(stream,
+				 "\tExpected %s to be false, but is true\n",
+				 unary_assert->condition);
+	kunit_assert_print_msg(assert, stream);
+}
+
+void kunit_ptr_not_err_assert_format(struct kunit_assert *assert,
+				     struct kunit_stream *stream)
+{
+	struct kunit_ptr_not_err_assert *ptr_assert = container_of(
+			assert, struct kunit_ptr_not_err_assert, assert);
+
+	kunit_base_assert_format(assert, stream);
+	if (!ptr_assert->value) {
+		kunit_stream_add(stream,
+				 "\tExpected %s is not null, but is\n",
+				 ptr_assert->text);
+	} else if (IS_ERR(ptr_assert->value)) {
+		kunit_stream_add(stream,
+				 "\tExpected %s is not error, but is: %ld\n",
+				 ptr_assert->text,
+				 PTR_ERR(ptr_assert->value));
+	}
+	kunit_assert_print_msg(assert, stream);
+}
+
+void kunit_binary_assert_format(struct kunit_assert *assert,
+				struct kunit_stream *stream)
+{
+	struct kunit_binary_assert *binary_assert = container_of(
+			assert, struct kunit_binary_assert, assert);
+
+	kunit_base_assert_format(assert, stream);
+	kunit_stream_add(stream,
+			 "\tExpected %s %s %s, but\n",
+			 binary_assert->left_text,
+			 binary_assert->operation,
+			 binary_assert->right_text);
+	kunit_stream_add(stream, "\t\t%s == %lld\n",
+			 binary_assert->left_text,
+			 binary_assert->left_value);
+	kunit_stream_add(stream, "\t\t%s == %lld",
+			 binary_assert->right_text,
+			 binary_assert->right_value);
+	kunit_assert_print_msg(assert, stream);
+}
+
+void kunit_binary_ptr_assert_format(struct kunit_assert *assert,
+				    struct kunit_stream *stream)
+{
+	struct kunit_binary_ptr_assert *binary_assert = container_of(
+			assert, struct kunit_binary_ptr_assert, assert);
+
+	kunit_base_assert_format(assert, stream);
+	kunit_stream_add(stream,
+			 "\tExpected %s %s %s, but\n",
+			 binary_assert->left_text,
+			 binary_assert->operation,
+			 binary_assert->right_text);
+	kunit_stream_add(stream, "\t\t%s == %pK\n",
+			 binary_assert->left_text,
+			 binary_assert->left_value);
+	kunit_stream_add(stream, "\t\t%s == %pK",
+			 binary_assert->right_text,
+			 binary_assert->right_value);
+	kunit_assert_print_msg(assert, stream);
+}
+
+void kunit_binary_str_assert_format(struct kunit_assert *assert,
+				    struct kunit_stream *stream)
+{
+	struct kunit_binary_str_assert *binary_assert = container_of(
+			assert, struct kunit_binary_str_assert, assert);
+
+	kunit_base_assert_format(assert, stream);
+	kunit_stream_add(stream,
+			 "\tExpected %s %s %s, but\n",
+			 binary_assert->left_text,
+			 binary_assert->operation,
+			 binary_assert->right_text);
+	kunit_stream_add(stream, "\t\t%s == %s\n",
+			 binary_assert->left_text,
+			 binary_assert->left_value);
+	kunit_stream_add(stream, "\t\t%s == %s",
+			 binary_assert->right_text,
+			 binary_assert->right_value);
+	kunit_assert_print_msg(assert, stream);
+}
+
+void kunit_mock_declaration_format(struct kunit_mock_declaration *declaration,
+				   struct kunit_stream *stream)
+{
+	int i;
+
+	kunit_stream_add(stream, "%s(", declaration->function_name);
+	for (i = 0; i < declaration->len; i++) {
+		mock_format_param(stream,
+				  declaration->type_names[i],
+				  declaration->params[i]);
+		if (i < declaration->len - 1)
+			kunit_stream_add(stream, ", ");
+	}
+	kunit_stream_add(stream, ")\n");
+}
+
+void kunit_mock_failed_match_format(struct kunit_mock_failed_match *match,
+				    struct kunit_stream *stream)
+{
+	struct kunit_matcher_result *result;
+	size_t i;
+
+	kunit_stream_add(stream,
+			 "Tried expectation: %s, but\n",
+			 match->expectation_text);
+	for (i = 0; i < match->matcher_list_len; i++) {
+		result = &match->matcher_list[i];
+		kunit_stream_add(stream, "\t");
+		result->assert.format(&result->assert, stream);
+		kunit_stream_add(stream, "\n");
+	}
+}
+
+void kunit_mock_no_match_format(struct kunit_assert *assert,
+				struct kunit_stream *stream)
+{
+	struct kunit_mock_assert *mock_assert = container_of(
+			assert, struct kunit_mock_assert, assert);
+	struct kunit_mock_no_match *no_match = container_of(
+			mock_assert, struct kunit_mock_no_match, assert);
+	struct kunit_mock_failed_match *expectation;
+
+	kunit_base_assert_format(assert, stream);
+	kunit_mock_declaration_format(&no_match->declaration, stream);
+
+	list_for_each_entry(expectation, &no_match->failed_match_list, node)
+		kunit_mock_failed_match_format(expectation, stream);
+}
diff --git a/kunit/mock.c b/kunit/mock.c
index ccb0abe111402..ab441a58a918c 100644
--- a/kunit/mock.c
+++ b/kunit/mock.c
@@ -269,9 +269,9 @@ struct mock_param_formatter *mock_find_formatter(const char *type_name)
 	return NULL;
 }
 
-static void mock_format_param(struct kunit_stream *stream,
-			      const char *type_name,
-			      const void *param)
+void mock_format_param(struct kunit_stream *stream,
+		       const char *type_name,
+		       const void *param)
 {
 	struct mock_param_formatter *formatter;
 
-- 
2.22.0.657.g960e92d24f-goog