Rework the assert_return() macro#375
Conversation
I think not needing to provide the same EXIT_FAILURE and being able to provide a message is good. The rename I think is not really needed. What's the problem with the term assert? IMO it's clearer than "ok". igt for example uses a bunch of igt_assert_*. ok() IMO is odd, but it maybe because I've never seen it used like this. Rust also has both Ok() and assert!(), which I think will add to the confusion.
I think a bulk one. If the message is optional, then the first patch is automated. Additional patches may follow later (maybe per compile unit) to add some good messages. |
There is nothing wrong with the term in itself. The snafu happens due to Other naming options:
Don't have a strong preference though.
Awesome, will do.
Do the example commits reach that bar, or shall I just drop them? |
|
@lucasdemarchi humble poke |
|
ts_expect() or ts_assert() would be fine IMO. Other places that use the "expect" terminology: GoogleTest and kunit... Both of them use semantics like
So for our uses I think it matches more the "assert" semantics |
|
Changes:
|
|
Will check the CI failures later today/tomorrow - the PR should be good for review though. |
evelikov
changed the title
|
From the failures:
|
lucasdemarchi
left a comment
lucasdemarchi
left a comment
There was a problem hiding this comment.
Just a nit on commit message. LGTM.
| __LINE__, __PRETTY_FUNCTION__); \ | ||
| return (r); \ | ||
| ERR("ASSERTION FAILED at %s:%d\n", __FILE__, __LINE__); \ | ||
| ERR("\t" #expr "\n"); \ |
There was a problem hiding this comment.
From commit message:
- embeds the expression into the error string, growing the file size
which is still true here, right? Also, for the testsuite the file size due to this would not be of great concern to point out.
There was a problem hiding this comment.
It meant that the expression string is separate from the boilerplate "foobar failed" error message, allowing the latter to be de-duplicated. Nevertheless, dropped since it's a very meh point.
With follow-up commit we'll rework/remove the macro, where the new version does not allow for providing the return value. Swap it for a manual ERR, printing a more useful error message. Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
The existing macro has a few shortcomings: - lowercase, non-prefixed so it can be confused with assert(3), which has varying behaviour depending on -DNDEBUG - the return value is constant across the project Introduce TS_ASSERT which is modelled after KUNIT_ASSERT, addressing the above and tweaking the output format. Existing instances of assert_return will be updated with a follow-up commit. Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Mechanical change, done with: sed -i "s/assert_return/TS_ASSERT/;s/, EXIT_FAILURE//" testsuite/*.c In addition, manually fixup the two multiline instances (test-strbuf.c and test-util.c) and remove the no longer used macro. Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Currently we have some inconsistencies across the tests: - using TS_ASSERT vs not - (non TS_ASSERT tests) leaking on error vs not In practical terms, we are not too worried about the leaks, since the test failure comes first. As such, convert all the tests to TS_ASSERT(). This means we loose the useful error messages in a few instances, which could be re-introduced at a later point alongside a TS_ASSERT_MSG() macro. Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
With all the EXIT_FAILURE instances done from the tests (only ones in tools and the test runner remain), lets drop the EXIT_SUCCESS ones. Functionally identical and arguably slightly neater. Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
2 participants
From the commit introducing it:
... and the follow-up commit:
Before going forward and porting more tests, would be great to hear if the direction is right.
If* it is, a couple of questions: