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@xxxxxxxxxx> wrote: > > > > Quoting Brendan Higgins (2019-07-16 11:52:01) > > > On Tue, Jul 16, 2019 at 10:50 AM Stephen Boyd <sboyd@xxxxxxxxxx> wrote: [...] > > Do you have a link to those earlier patches? > > This is the first patchset: > > https://www.mail-archive.com/linux-kernel@xxxxxxxxxxxxxxx/msg1788057.html > > In particular you can see the code for matching functions here: > > https://www.mail-archive.com/linux-kernel@xxxxxxxxxxxxxxx/msg1788073.html > > And parameter matching code here: > > https://www.mail-archive.com/linux-kernel@xxxxxxxxxxxxxxx/msg1788072.html > > https://www.mail-archive.com/linux-kernel@xxxxxxxxxxxxxxx/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@xxxxxxxxxx> 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@xxxxxxxxxx> + */ + +#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@xxxxxxxxxx> + */ +#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
