Quoting Brendan Higgins (2019-07-16 11:52:01)

On Tue, Jul 16, 2019 at 10:50 AM Stephen Boyd [off-list ref] wrote:

quoted

quoted

The only hypothetical case where this can't be done is a complicated
assertion or expectation that does more than one check and can't be
written as a function that dumps out what went wrong. Is this a real
problem? Maybe such an assertion should just open code that logic so we
don't have to build up a string for all the other simple cases.

I have some expectations in follow up patchsets for which I created a
set of composable matchers for matching structures and function calls
that by their nature cannot be written as a single function. The
matcher thing is a bit speculative, I know, but for any kind of
function call matching, you need to store a record of functions you
are expecting to have called and then each one needs to have a set of
expectations defined by the user; I don't think there is a way to do
that that doesn't involve having multiple separate functions each
having some information useful to constructing the message.

I know the code in question isn't in this patchset; the function
matching code was in one of the earlier versions of the RFC, but I
dropped it to make this patchset smaller and more manageable. So I get
it if you would like me to drop it and add it back in when I try to
get the function and structure matching stuff in, but I would really
prefer to keep it as is if you don't care too much.

Do you have a link to those earlier patches?

quoted

It seems far simpler to get rid of the string stream API and just have a
struct for this.

        struct kunit_fail_msg {
                const char *line;
                const char *file;
                const char *func;
                const char *msg;
        };

Then you can have the assertion macros create this on the stack (with
another macro?).

        #define DEFINE_KUNIT_FAIL_MSG(name, _msg) \
                struct kunit_fail_msg name = { \
                        .line =  __LINE__, \
                        .file = __FILE__, \
                        .func = __func__, \
                        .msg = _msg, \
                }

I don't want to derail this whole series on this topic, but it seems
like a bunch of code is there to construct this same set of information
over and over again into a buffer a little bit at a time and then throw
it away when nothing fails just because we may want to support the case
where we have some unstructured data to inform the user about.

Yeah, that's fair. I think there are a number of improvements to be
made with how the expectations are defined other than that, but I was
hoping I could do that after this patchset is merged. I just figured
with the kinds of things I would like to do, it would lead to a whole
new round of discussion.

In either case, I think I would still like to use the `struct
kunit_stream` as part of the interface to share the failure message
with the test case runner code in test.c, at least eventually, so that
I only have to have one way to receive data from expectations, but I
think I can do that and still do what you suggest by just constructing
the kunit_stream at the end of expectations where it is feasible.

All in all I agree with what you are saying, but I would rather do it
as a follow up possibly once we have some more code on the table. I
could just see this opening up a whole new can of worms where we
debate about exactly how expectations and assertions work for another
several months, only to rip it all out shortly there after. I know
that's how these things go, but that's my preference.

I can do what you suggest if you feel strongly about it, but I would
prefer to hold off until later. It's your call.

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.

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.

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.

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(...)
	}

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
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.