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