| Message ID | 20230330220506.1399796-1-rmoar@google.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | [v1] kunit: add tests for using current KUnit test field | expand |
I've got a few minor comments below, but this otherwise looks good. I like the idea of testing knuit_fail_current_test(). On Thu, Mar 30, 2023 at 3:05 PM Rae Moar <rmoar@google.com> wrote: > > +static void kunit_current_kunit_test_field(struct kunit *test) > +{ > + struct kunit *current_test; > + > + /* Check to ensure the result of current->kunit_test > + * is equivalent to current test. > + */ > + current_test = current->kunit_test; > + KUNIT_EXPECT_PTR_EQ(test, test, current_test); Perhaps we can combine this and the next test case down to static void kunit_current_test(struct kunit *test) { /* There are two different ways of getting the current test */ KUNIT_EXPECT_PTR_EQ(test, test, current->kunit_test); KUNIT_EXPECT_PTR_EQ(test, test, kunit_get_current_test()); } ? > +} > + > +static void kunit_current_get_current_test(struct kunit *test) > +{ > + struct kunit *current_test1, *current_test2; > + > + /* Check to ensure the result of kunit_get_current_test() > + * is equivalent to current test. > + */ > + current_test1 = kunit_get_current_test(); > + KUNIT_EXPECT_PTR_EQ(test, test, current_test1); > + > + /* Check to ensure the result of kunit_get_current_test() > + * is equivalent to current->kunit_test. > + */ > + current_test2 = current->kunit_test; > + KUNIT_EXPECT_PTR_EQ(test, current_test1, current_test2); > +} > + > +static void kunit_current_fail_current_test(struct kunit *test) > +{ > + struct kunit fake; > + > + /* Initialize fake test and set as current->kunit_test. */ Nit: I think the code is self-explanatory enough that we can drop this comment. > + kunit_init_test(&fake, "fake test", NULL); > + KUNIT_EXPECT_EQ(test, fake.status, KUNIT_SUCCESS); > + current->kunit_test = &fake; > + > + /* Fail current test and expect status of fake test to be failed. */ Nit: I think this comment could also be dropped or maybe shortened to kunit_fail_current_test("This should make `fake` fail"); or /* Now kunit_fail_current_test() should modify `fake`, not `test` */ kunit_fail_current_test("This should make `fake` fail"); > + kunit_fail_current_test("This test is supposed to fail."); > + KUNIT_EXPECT_EQ(test, fake.status, (enum kunit_status)KUNIT_FAILURE); > + Hmm, should we try calling kunit_cleanup(&fake); ? Right now this does resource cleanups, but we might have other state to cleanup for our `fake` test object in the future. Daniel
On Fri, 31 Mar 2023 at 06:21, 'Daniel Latypov' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > I've got a few minor comments below, but this otherwise looks good. > I like the idea of testing knuit_fail_current_test(). > > > On Thu, Mar 30, 2023 at 3:05 PM Rae Moar <rmoar@google.com> wrote: > > > > +static void kunit_current_kunit_test_field(struct kunit *test) > > +{ > > + struct kunit *current_test; > > + > > + /* Check to ensure the result of current->kunit_test > > + * is equivalent to current test. > > + */ > > + current_test = current->kunit_test; > > + KUNIT_EXPECT_PTR_EQ(test, test, current_test); > > Perhaps we can combine this and the next test case down to > static void kunit_current_test(struct kunit *test) { > /* There are two different ways of getting the current test */ > KUNIT_EXPECT_PTR_EQ(test, test, current->kunit_test); > KUNIT_EXPECT_PTR_EQ(test, test, kunit_get_current_test()); > } > ? > Agreed: checking current->kunit_test twice feels a bit odd. > > +} > > + > > +static void kunit_current_get_current_test(struct kunit *test) > > +{ > > + struct kunit *current_test1, *current_test2; > > + > > + /* Check to ensure the result of kunit_get_current_test() > > + * is equivalent to current test. > > + */ > > + current_test1 = kunit_get_current_test(); > > + KUNIT_EXPECT_PTR_EQ(test, test, current_test1); > > + > > + /* Check to ensure the result of kunit_get_current_test() > > + * is equivalent to current->kunit_test. > > + */ > > + current_test2 = current->kunit_test; > > + KUNIT_EXPECT_PTR_EQ(test, current_test1, current_test2); > > > +} > > + > > +static void kunit_current_fail_current_test(struct kunit *test) > > +{ > > + struct kunit fake; > > + > > + /* Initialize fake test and set as current->kunit_test. */ > > Nit: I think the code is self-explanatory enough that we can drop this comment. > > > + kunit_init_test(&fake, "fake test", NULL); > > + KUNIT_EXPECT_EQ(test, fake.status, KUNIT_SUCCESS); > > + current->kunit_test = &fake; > > + > > + /* Fail current test and expect status of fake test to be failed. */ > > Nit: I think this comment could also be dropped or maybe shortened to > kunit_fail_current_test("This should make `fake` fail"); > > or > /* Now kunit_fail_current_test() should modify `fake`, not `test` */ > kunit_fail_current_test("This should make `fake` fail"); > > > + kunit_fail_current_test("This test is supposed to fail."); > > + KUNIT_EXPECT_EQ(test, fake.status, (enum kunit_status)KUNIT_FAILURE); > > + > > Hmm, should we try calling > kunit_cleanup(&fake); > ? > > Right now this does resource cleanups, but we might have other state > to cleanup for our `fake` test object in the future. I could go either way here. We currently don't do this with the other status tests (kunit_status), only with the resource ones. But it doesn't hurt to add it... > > Daniel > > -- > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/CAGS_qxqNwVcymkG6-8Kv72oZc9aDqjFjBBmjr%2Bf%2BmOVKT1bGvA%40mail.gmail.com.
On Thu, Mar 30, 2023 at 6:21 PM 'Daniel Latypov' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > I've got a few minor comments below, but this otherwise looks good. > I like the idea of testing knuit_fail_current_test(). > > > On Thu, Mar 30, 2023 at 3:05 PM Rae Moar <rmoar@google.com> wrote: > > > > +static void kunit_current_kunit_test_field(struct kunit *test) > > +{ > > + struct kunit *current_test; > > + > > + /* Check to ensure the result of current->kunit_test > > + * is equivalent to current test. > > + */ > > + current_test = current->kunit_test; > > + KUNIT_EXPECT_PTR_EQ(test, test, current_test); > > Perhaps we can combine this and the next test case down to > static void kunit_current_test(struct kunit *test) { > /* There are two different ways of getting the current test */ > KUNIT_EXPECT_PTR_EQ(test, test, current->kunit_test); > KUNIT_EXPECT_PTR_EQ(test, test, kunit_get_current_test()); > } > ? Hi Daniel! Yes, I would be happy to combine these for v2. I might want to alter that proposed comment slightly. "Two different ways" seems a bit unclear to me. Maybe: Check results of both current->kunit_test and kunit_get_current_test() are equivalent to current test. What do you think? I might send out a v2 with a proposed comment. > > > +} > > + > > +static void kunit_current_get_current_test(struct kunit *test) > > +{ > > + struct kunit *current_test1, *current_test2; > > + > > + /* Check to ensure the result of kunit_get_current_test() > > + * is equivalent to current test. > > + */ > > + current_test1 = kunit_get_current_test(); > > + KUNIT_EXPECT_PTR_EQ(test, test, current_test1); > > + > > + /* Check to ensure the result of kunit_get_current_test() > > + * is equivalent to current->kunit_test. > > + */ > > + current_test2 = current->kunit_test; > > + KUNIT_EXPECT_PTR_EQ(test, current_test1, current_test2); > > > +} > > + > > +static void kunit_current_fail_current_test(struct kunit *test) > > +{ > > + struct kunit fake; > > + > > + /* Initialize fake test and set as current->kunit_test. */ > > Nit: I think the code is self-explanatory enough that we can drop this comment. > I agree the "initialize fake test" comment is self-explanatory. But if we keep the comment regarding resetting the current test, I think we should mark when we set the test as a fake with a comment as well. > > + kunit_init_test(&fake, "fake test", NULL); > > + KUNIT_EXPECT_EQ(test, fake.status, KUNIT_SUCCESS); > > + current->kunit_test = &fake; > > + > > + /* Fail current test and expect status of fake test to be failed. */ > > Nit: I think this comment could also be dropped or maybe shortened to > kunit_fail_current_test("This should make `fake` fail"); > This first option seems good to me. > or > /* Now kunit_fail_current_test() should modify `fake`, not `test` */ > kunit_fail_current_test("This should make `fake` fail"); > > > + kunit_fail_current_test("This test is supposed to fail."); > > + KUNIT_EXPECT_EQ(test, fake.status, (enum kunit_status)KUNIT_FAILURE); > > + > > Hmm, should we try calling > kunit_cleanup(&fake); > ? > > Right now this does resource cleanups, but we might have other state > to cleanup for our `fake` test object in the future. > I would be fine to add this here if it is wanted. Thanks Daniel for the comments! Rae > Daniel > > -- > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/CAGS_qxqNwVcymkG6-8Kv72oZc9aDqjFjBBmjr%2Bf%2BmOVKT1bGvA%40mail.gmail.com.
On Mon, Apr 3, 2023 at 12:31 PM Rae Moar <rmoar@google.com> wrote: > > On Thu, Mar 30, 2023 at 6:21 PM 'Daniel Latypov' via KUnit Development > <kunit-dev@googlegroups.com> wrote: > > > > I've got a few minor comments below, but this otherwise looks good. > > I like the idea of testing knuit_fail_current_test(). > > > > > > On Thu, Mar 30, 2023 at 3:05 PM Rae Moar <rmoar@google.com> wrote: > > > > > > +static void kunit_current_kunit_test_field(struct kunit *test) > > > +{ > > > + struct kunit *current_test; > > > + > > > + /* Check to ensure the result of current->kunit_test > > > + * is equivalent to current test. > > > + */ > > > + current_test = current->kunit_test; > > > + KUNIT_EXPECT_PTR_EQ(test, test, current_test); > > > > Perhaps we can combine this and the next test case down to > > static void kunit_current_test(struct kunit *test) { > > /* There are two different ways of getting the current test */ > > KUNIT_EXPECT_PTR_EQ(test, test, current->kunit_test); > > KUNIT_EXPECT_PTR_EQ(test, test, kunit_get_current_test()); > > } > > ? > > Hi Daniel! > > Yes, I would be happy to combine these for v2. I might want to alter > that proposed comment slightly. "Two different ways" seems a bit > unclear to me. Maybe: Check results of both current->kunit_test and > kunit_get_current_test() are equivalent to current test. What do you > think? I might send out a v2 with a proposed comment. What you went with in v2 works for me. I'll take a look at the other changes in v2. Thanks! Daniel
diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c index b63595d3e241..91984b92c916 100644 --- a/lib/kunit/kunit-test.c +++ b/lib/kunit/kunit-test.c @@ -6,6 +6,7 @@ * Author: Brendan Higgins <brendanhiggins@google.com> */ #include <kunit/test.h> +#include <kunit/test-bug.h> #include "try-catch-impl.h" @@ -532,7 +533,65 @@ static struct kunit_suite kunit_status_test_suite = { .test_cases = kunit_status_test_cases, }; +static void kunit_current_kunit_test_field(struct kunit *test) +{ + struct kunit *current_test; + + /* Check to ensure the result of current->kunit_test + * is equivalent to current test. + */ + current_test = current->kunit_test; + KUNIT_EXPECT_PTR_EQ(test, test, current_test); +} + +static void kunit_current_get_current_test(struct kunit *test) +{ + struct kunit *current_test1, *current_test2; + + /* Check to ensure the result of kunit_get_current_test() + * is equivalent to current test. + */ + current_test1 = kunit_get_current_test(); + KUNIT_EXPECT_PTR_EQ(test, test, current_test1); + + /* Check to ensure the result of kunit_get_current_test() + * is equivalent to current->kunit_test. + */ + current_test2 = current->kunit_test; + KUNIT_EXPECT_PTR_EQ(test, current_test1, current_test2); +} + +static void kunit_current_fail_current_test(struct kunit *test) +{ + struct kunit fake; + + /* Initialize fake test and set as current->kunit_test. */ + kunit_init_test(&fake, "fake test", NULL); + KUNIT_EXPECT_EQ(test, fake.status, KUNIT_SUCCESS); + current->kunit_test = &fake; + + /* Fail current test and expect status of fake test to be failed. */ + kunit_fail_current_test("This test is supposed to fail."); + KUNIT_EXPECT_EQ(test, fake.status, (enum kunit_status)KUNIT_FAILURE); + + /* Reset current->kunit_test to current test. */ + current->kunit_test = test; +} + +static struct kunit_case kunit_current_test_cases[] = { + KUNIT_CASE(kunit_current_kunit_test_field), + KUNIT_CASE(kunit_current_get_current_test), + KUNIT_CASE(kunit_current_fail_current_test), + {} +}; + +static struct kunit_suite kunit_current_test_suite = { + .name = "kunit_current", + .test_cases = kunit_current_test_cases, +}; + kunit_test_suites(&kunit_try_catch_test_suite, &kunit_resource_test_suite, - &kunit_log_test_suite, &kunit_status_test_suite); + &kunit_log_test_suite, &kunit_status_test_suite, + &kunit_current_test_suite); MODULE_LICENSE("GPL v2");
Create test suite called "kunit_current" to add test coverage for the use of current->kunit_test, which returns the current KUnit test. Add three test cases: - kunit_current_kunit_test_field to test the use of current->kunit_test. - kunit_current_get_current_test to test the method kunit_get_current_test(), which utilizes current->kunit_test. - kunit_current_fail_current_test to test the method kunit_fail_current_test(), which utilizes current->kunit_test. Signed-off-by: Rae Moar <rmoar@google.com> --- lib/kunit/kunit-test.c | 61 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 60 insertions(+), 1 deletion(-) base-commit: 7232282dd47cce6a780c9414bd9baccf232c7686