| Message ID | 20221001002638.2881842-3-dlatypov@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 97d453bc4007d4ac148c2ba89904026612b91ec9 |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | kunit: more assertion reworking | expand |
On Sat, Oct 1, 2022 at 8:26 AM 'Daniel Latypov' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > Context: > Currently this macro's name, KUNIT_ASSERTION conflicts with the name of > an enum whose values are {KUNIT_EXPECTATION, KUNIT_ASSERTION}. > > It's hard to think of a better name for the enum, so rename this macro. > It's also a bit strange that the macro might do nothing depending on the > boolean argument `pass`. Why not have callers check themselves? > > This patch: > Moves the pass/fail checking into the callers of KUNIT_ASSERTION, so now > we only call it when the check has failed. > Then we rename the macro the _KUNIT_FAILED() to reflect the new > semantics. > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > --- Looks good to me. I can't say the name _KUNIT_FAILED() feels perfect to me, but I can't think of anything better, either. We've not used a leading underscore for internal macros much thus far, as well, though I've no personal objections to starting. Regardless, let's get this in. Reviewed-by: David Gow <davidgow@google.com> Cheers, -- David > include/kunit/test.h | 123 +++++++++++++++++++++++-------------------- > 1 file changed, 65 insertions(+), 58 deletions(-) > > diff --git a/include/kunit/test.h b/include/kunit/test.h > index 3476549106f7..fec437c8a2b7 100644 > --- a/include/kunit/test.h > +++ b/include/kunit/test.h > @@ -475,30 +475,27 @@ void kunit_do_failed_assertion(struct kunit *test, > assert_format_t assert_format, > const char *fmt, ...); > > -#define KUNIT_ASSERTION(test, assert_type, pass, assert_class, assert_format, INITIALIZER, fmt, ...) do { \ > - if (unlikely(!(pass))) { \ > - static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \ > - struct assert_class __assertion = INITIALIZER; \ > - kunit_do_failed_assertion(test, \ > - &__loc, \ > - assert_type, \ > - &__assertion.assert, \ > - assert_format, \ > - fmt, \ > - ##__VA_ARGS__); \ > - } \ > +#define _KUNIT_FAILED(test, assert_type, assert_class, assert_format, INITIALIZER, fmt, ...) do { \ > + static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \ > + struct assert_class __assertion = INITIALIZER; \ > + kunit_do_failed_assertion(test, \ > + &__loc, \ > + assert_type, \ > + &__assertion.assert, \ > + assert_format, \ > + fmt, \ > + ##__VA_ARGS__); \ > } while (0) > > > #define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) \ > - KUNIT_ASSERTION(test, \ > - assert_type, \ > - false, \ > - kunit_fail_assert, \ > - kunit_fail_assert_format, \ > - {}, \ > - fmt, \ > - ##__VA_ARGS__) > + _KUNIT_FAILED(test, \ > + assert_type, \ > + kunit_fail_assert, \ > + kunit_fail_assert_format, \ > + {}, \ > + fmt, \ > + ##__VA_ARGS__) > > /** > * KUNIT_FAIL() - Always causes a test to fail when evaluated. > @@ -523,15 +520,19 @@ void kunit_do_failed_assertion(struct kunit *test, > expected_true, \ > fmt, \ > ...) \ > - KUNIT_ASSERTION(test, \ > - assert_type, \ > - !!(condition) == !!expected_true, \ > - kunit_unary_assert, \ > - kunit_unary_assert_format, \ > - KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \ > - expected_true), \ > - fmt, \ > - ##__VA_ARGS__) > +do { \ > + if (likely(!!(condition) == !!expected_true)) \ > + break; \ > + \ > + _KUNIT_FAILED(test, \ > + assert_type, \ > + kunit_unary_assert, \ > + kunit_unary_assert_format, \ > + KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \ > + expected_true), \ > + fmt, \ > + ##__VA_ARGS__); \ > +} while (0) > > #define KUNIT_TRUE_MSG_ASSERTION(test, assert_type, condition, fmt, ...) \ > KUNIT_UNARY_ASSERTION(test, \ > @@ -581,16 +582,18 @@ do { \ > .right_text = #right, \ > }; \ > \ > - KUNIT_ASSERTION(test, \ > - assert_type, \ > - __left op __right, \ > - assert_class, \ > - format_func, \ > - KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \ > - __left, \ > - __right), \ > - fmt, \ > - ##__VA_ARGS__); \ > + if (likely(__left op __right)) \ > + break; \ > + \ > + _KUNIT_FAILED(test, \ > + assert_type, \ > + assert_class, \ > + format_func, \ > + KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \ > + __left, \ > + __right), \ > + fmt, \ > + ##__VA_ARGS__); \ > } while (0) > > #define KUNIT_BINARY_INT_ASSERTION(test, \ > @@ -639,16 +642,19 @@ do { \ > .right_text = #right, \ > }; \ > \ > - KUNIT_ASSERTION(test, \ > - assert_type, \ > - strcmp(__left, __right) op 0, \ > - kunit_binary_str_assert, \ > - kunit_binary_str_assert_format, \ > - KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \ > - __left, \ > - __right), \ > - fmt, \ > - ##__VA_ARGS__); \ > + if (likely(strcmp(__left, __right) op 0)) \ > + break; \ > + \ > + \ > + _KUNIT_FAILED(test, \ > + assert_type, \ > + kunit_binary_str_assert, \ > + kunit_binary_str_assert_format, \ > + KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \ > + __left, \ > + __right), \ > + fmt, \ > + ##__VA_ARGS__); \ > } while (0) > > #define KUNIT_PTR_NOT_ERR_OR_NULL_MSG_ASSERTION(test, \ > @@ -659,15 +665,16 @@ do { \ > do { \ > const typeof(ptr) __ptr = (ptr); \ > \ > - KUNIT_ASSERTION(test, \ > - assert_type, \ > - !IS_ERR_OR_NULL(__ptr), \ > - kunit_ptr_not_err_assert, \ > - kunit_ptr_not_err_assert_format, \ > - KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, \ > - __ptr), \ > - fmt, \ > - ##__VA_ARGS__); \ > + if (!IS_ERR_OR_NULL(__ptr)) \ > + break; \ > + \ > + _KUNIT_FAILED(test, \ > + assert_type, \ > + kunit_ptr_not_err_assert, \ > + kunit_ptr_not_err_assert_format, \ > + KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, __ptr), \ > + fmt, \ > + ##__VA_ARGS__); \ > } while (0) > > /** > -- > 2.38.0.rc1.362.ged0d419d3c-goog > > -- > 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/20221001002638.2881842-3-dlatypov%40google.com.
On Fri, Sep 30, 2022 at 8:26 PM David Gow <davidgow@google.com> wrote: > > On Sat, Oct 1, 2022 at 8:26 AM 'Daniel Latypov' via KUnit Development > <kunit-dev@googlegroups.com> wrote: > > > > Context: > > Currently this macro's name, KUNIT_ASSERTION conflicts with the name of > > an enum whose values are {KUNIT_EXPECTATION, KUNIT_ASSERTION}. > > > > It's hard to think of a better name for the enum, so rename this macro. > > It's also a bit strange that the macro might do nothing depending on the > > boolean argument `pass`. Why not have callers check themselves? > > > > This patch: > > Moves the pass/fail checking into the callers of KUNIT_ASSERTION, so now > > we only call it when the check has failed. > > Then we rename the macro the _KUNIT_FAILED() to reflect the new > > semantics. > > > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > > --- > > Looks good to me. I can't say the name _KUNIT_FAILED() feels perfect > to me, but I can't think of anything better, either. We've not used a > leading underscore for internal macros much thus far, as well, though > I've no personal objections to starting. Yeah, I also didn't add a leading underscore on the new KUNIT_INIT_ASSERT() macro elsewhere in this series. So I'm not necessarily proposing that we should start doing so here. It feels like that KUNIT_FAILED is far too similar to the enum 55 enum kunit_status { 56 KUNIT_SUCCESS, 57 KUNIT_FAILURE, 58 KUNIT_SKIPPED, 59 }; I.e. we'd be remove one naming conflict between a macro and enum, but basically introducing a new one in its place :P Tbh, I was originally going to have this patch just be s/KUNIT_ASSERTION()/_KUNIT_ASSERTION() to reduce the conflict. But I figured we could reduce the number of arguments to the macro (drop `pass`) and have a reason to give it a different name. I'm also not entirely convinced about _KUNIT_FAILED(), but I haven't had any significantly better ideas since I sent the RFC in May. Daniel
On Sat, Oct 1, 2022 at 11:50 AM 'Daniel Latypov' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > On Fri, Sep 30, 2022 at 8:26 PM David Gow <davidgow@google.com> wrote: > > > > On Sat, Oct 1, 2022 at 8:26 AM 'Daniel Latypov' via KUnit Development > > <kunit-dev@googlegroups.com> wrote: > > > > > > Context: > > > Currently this macro's name, KUNIT_ASSERTION conflicts with the name of > > > an enum whose values are {KUNIT_EXPECTATION, KUNIT_ASSERTION}. > > > > > > It's hard to think of a better name for the enum, so rename this macro. > > > It's also a bit strange that the macro might do nothing depending on the > > > boolean argument `pass`. Why not have callers check themselves? > > > > > > This patch: > > > Moves the pass/fail checking into the callers of KUNIT_ASSERTION, so now > > > we only call it when the check has failed. > > > Then we rename the macro the _KUNIT_FAILED() to reflect the new > > > semantics. > > > > > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > > > --- > > > > Looks good to me. I can't say the name _KUNIT_FAILED() feels perfect > > to me, but I can't think of anything better, either. We've not used a > > leading underscore for internal macros much thus far, as well, though > > I've no personal objections to starting. > > Yeah, I also didn't add a leading underscore on the new > KUNIT_INIT_ASSERT() macro elsewhere in this series. > So I'm not necessarily proposing that we should start doing so here. Yeah: I noticed that. In general, I think I come down slightly on the side of avoiding leading underscores. (And there's also the debate about whether to have one or two, as while two underscores is nominally "reserved for the system", the kernel uses it a lot -- probably because it considers itself "the system"). So I'd tend to lean away from making all of our "internal" macros start with underscores. > > It feels like that KUNIT_FAILED is far too similar to the enum > 55 enum kunit_status { > 56 KUNIT_SUCCESS, > 57 KUNIT_FAILURE, > 58 KUNIT_SKIPPED, > 59 }; > > I.e. we'd be remove one naming conflict between a macro and enum, but > basically introducing a new one in its place :P > Tbh, I was originally going to have this patch just be > s/KUNIT_ASSERTION()/_KUNIT_ASSERTION() to reduce the conflict. > But I figured we could reduce the number of arguments to the macro > (drop `pass`) and have a reason to give it a different name. > > I'm also not entirely convinced about _KUNIT_FAILED(), but I haven't > had any significantly better ideas since I sent the RFC in May. Agreed: particularly since SKIPPED would seem to pair better with FAILED than FAILURE, so they conflict quite a bit. Let's stick with what we have in this change, and we can change it later if someone comes up with a drastically better name. Cheers, -- David
diff --git a/include/kunit/test.h b/include/kunit/test.h index 3476549106f7..fec437c8a2b7 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -475,30 +475,27 @@ void kunit_do_failed_assertion(struct kunit *test, assert_format_t assert_format, const char *fmt, ...); -#define KUNIT_ASSERTION(test, assert_type, pass, assert_class, assert_format, INITIALIZER, fmt, ...) do { \ - if (unlikely(!(pass))) { \ - static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \ - struct assert_class __assertion = INITIALIZER; \ - kunit_do_failed_assertion(test, \ - &__loc, \ - assert_type, \ - &__assertion.assert, \ - assert_format, \ - fmt, \ - ##__VA_ARGS__); \ - } \ +#define _KUNIT_FAILED(test, assert_type, assert_class, assert_format, INITIALIZER, fmt, ...) do { \ + static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \ + struct assert_class __assertion = INITIALIZER; \ + kunit_do_failed_assertion(test, \ + &__loc, \ + assert_type, \ + &__assertion.assert, \ + assert_format, \ + fmt, \ + ##__VA_ARGS__); \ } while (0) #define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) \ - KUNIT_ASSERTION(test, \ - assert_type, \ - false, \ - kunit_fail_assert, \ - kunit_fail_assert_format, \ - {}, \ - fmt, \ - ##__VA_ARGS__) + _KUNIT_FAILED(test, \ + assert_type, \ + kunit_fail_assert, \ + kunit_fail_assert_format, \ + {}, \ + fmt, \ + ##__VA_ARGS__) /** * KUNIT_FAIL() - Always causes a test to fail when evaluated. @@ -523,15 +520,19 @@ void kunit_do_failed_assertion(struct kunit *test, expected_true, \ fmt, \ ...) \ - KUNIT_ASSERTION(test, \ - assert_type, \ - !!(condition) == !!expected_true, \ - kunit_unary_assert, \ - kunit_unary_assert_format, \ - KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \ - expected_true), \ - fmt, \ - ##__VA_ARGS__) +do { \ + if (likely(!!(condition) == !!expected_true)) \ + break; \ + \ + _KUNIT_FAILED(test, \ + assert_type, \ + kunit_unary_assert, \ + kunit_unary_assert_format, \ + KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \ + expected_true), \ + fmt, \ + ##__VA_ARGS__); \ +} while (0) #define KUNIT_TRUE_MSG_ASSERTION(test, assert_type, condition, fmt, ...) \ KUNIT_UNARY_ASSERTION(test, \ @@ -581,16 +582,18 @@ do { \ .right_text = #right, \ }; \ \ - KUNIT_ASSERTION(test, \ - assert_type, \ - __left op __right, \ - assert_class, \ - format_func, \ - KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \ - __left, \ - __right), \ - fmt, \ - ##__VA_ARGS__); \ + if (likely(__left op __right)) \ + break; \ + \ + _KUNIT_FAILED(test, \ + assert_type, \ + assert_class, \ + format_func, \ + KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \ + __left, \ + __right), \ + fmt, \ + ##__VA_ARGS__); \ } while (0) #define KUNIT_BINARY_INT_ASSERTION(test, \ @@ -639,16 +642,19 @@ do { \ .right_text = #right, \ }; \ \ - KUNIT_ASSERTION(test, \ - assert_type, \ - strcmp(__left, __right) op 0, \ - kunit_binary_str_assert, \ - kunit_binary_str_assert_format, \ - KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \ - __left, \ - __right), \ - fmt, \ - ##__VA_ARGS__); \ + if (likely(strcmp(__left, __right) op 0)) \ + break; \ + \ + \ + _KUNIT_FAILED(test, \ + assert_type, \ + kunit_binary_str_assert, \ + kunit_binary_str_assert_format, \ + KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \ + __left, \ + __right), \ + fmt, \ + ##__VA_ARGS__); \ } while (0) #define KUNIT_PTR_NOT_ERR_OR_NULL_MSG_ASSERTION(test, \ @@ -659,15 +665,16 @@ do { \ do { \ const typeof(ptr) __ptr = (ptr); \ \ - KUNIT_ASSERTION(test, \ - assert_type, \ - !IS_ERR_OR_NULL(__ptr), \ - kunit_ptr_not_err_assert, \ - kunit_ptr_not_err_assert_format, \ - KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, \ - __ptr), \ - fmt, \ - ##__VA_ARGS__); \ + if (!IS_ERR_OR_NULL(__ptr)) \ + break; \ + \ + _KUNIT_FAILED(test, \ + assert_type, \ + kunit_ptr_not_err_assert, \ + kunit_ptr_not_err_assert_format, \ + KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, __ptr), \ + fmt, \ + ##__VA_ARGS__); \ } while (0) /**
Context: Currently this macro's name, KUNIT_ASSERTION conflicts with the name of an enum whose values are {KUNIT_EXPECTATION, KUNIT_ASSERTION}. It's hard to think of a better name for the enum, so rename this macro. It's also a bit strange that the macro might do nothing depending on the boolean argument `pass`. Why not have callers check themselves? This patch: Moves the pass/fail checking into the callers of KUNIT_ASSERTION, so now we only call it when the check has failed. Then we rename the macro the _KUNIT_FAILED() to reflect the new semantics. Signed-off-by: Daniel Latypov <dlatypov@google.com> --- include/kunit/test.h | 123 +++++++++++++++++++++++-------------------- 1 file changed, 65 insertions(+), 58 deletions(-)