| Message ID | 983be396-f47c-4573-8c33-af8367f8ddbe@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] t-strvec: use test_msg() | expand |
On Fri, Jul 05, 2024 at 07:03:36PM +0200, René Scharfe wrote: > diff --git a/t/unit-tests/t-strvec.c b/t/unit-tests/t-strvec.c > index d4615ab06d..236203af61 100644 > --- a/t/unit-tests/t-strvec.c > +++ b/t/unit-tests/t-strvec.c > @@ -17,12 +17,12 @@ static void check_strvec_loc(const char *loc, struct strvec *vec, ...) > break; > > if (!check_uint(vec->nr, >, nr) || > - !check_uint(vec->alloc, >, nr) || > - !check_str(vec->v[nr], str)) { > - struct strbuf msg = STRBUF_INIT; > - strbuf_addf(&msg, "strvec index %"PRIuMAX, (uintmax_t) nr); > - test_assert(loc, msg.buf, 0); > - strbuf_release(&msg); > + !check_uint(vec->alloc, >, nr)) { > + va_end(ap); > + return; > + } > + if (!check_str(vec->v[nr], str)) { > + test_msg(" nr: %"PRIuMAX, (uintmax_t)nr); > va_end(ap); > return; > } The "loc" parameter to the function is now unused. Should it be removed, or is it a bug that we are no longer reporting the caller's location? Should we be using check_str_loc() in the post-image? -Peff
Am 09.07.24 um 13:32 schrieb Jeff King: > On Fri, Jul 05, 2024 at 07:03:36PM +0200, René Scharfe wrote: > >> diff --git a/t/unit-tests/t-strvec.c b/t/unit-tests/t-strvec.c >> index d4615ab06d..236203af61 100644 >> --- a/t/unit-tests/t-strvec.c >> +++ b/t/unit-tests/t-strvec.c >> @@ -17,12 +17,12 @@ static void check_strvec_loc(const char *loc, struct strvec *vec, ...) >> break; >> >> if (!check_uint(vec->nr, >, nr) || >> - !check_uint(vec->alloc, >, nr) || >> - !check_str(vec->v[nr], str)) { >> - struct strbuf msg = STRBUF_INIT; >> - strbuf_addf(&msg, "strvec index %"PRIuMAX, (uintmax_t) nr); >> - test_assert(loc, msg.buf, 0); >> - strbuf_release(&msg); >> + !check_uint(vec->alloc, >, nr)) { >> + va_end(ap); >> + return; >> + } >> + if (!check_str(vec->v[nr], str)) { >> + test_msg(" nr: %"PRIuMAX, (uintmax_t)nr); >> va_end(ap); >> return; >> } > > The "loc" parameter to the function is now unused. Should it be removed, > or is it a bug that we are no longer reporting the caller's location? It's a bug. If only there was a way to detect such an unused parameter automatically.. ;-> > Should we be using check_str_loc() in the post-image? Yes, and check_uint_loc() and check_pointer_eq_loc() as well. Which would be a pain. Or we drag everything into the macro check_strvec and get the caller's line number for free. René
On Sun, Jul 14, 2024 at 12:17:09PM +0200, René Scharfe wrote: > > Should we be using check_str_loc() in the post-image? > > Yes, and check_uint_loc() and check_pointer_eq_loc() as well. Which > would be a pain. Or we drag everything into the macro check_strvec and > get the caller's line number for free. Is it that big of a pain? It's mostly just passing "loc" along to the relative functions. To me it is more a problem that it is super easy to forget. Are the unit tests themselves multi-threaded within a single program? I'd think not. In which case, I kind of wonder if a simpler pattern would be to just set a global "location" variable (probably as a stack of strings) that all of the individual check functions used. And then we could set it once at the top-level of the test which wants its location reported, and any helper functions that get called in the middle would not have to care. And existing check_foo() functions could use the current file/line location if the stack is empty (so code that isn't using helper functions can remain unaffected). -Peff
Am 16.07.24 um 03:43 schrieb Jeff King: > On Sun, Jul 14, 2024 at 12:17:09PM +0200, René Scharfe wrote: > >>> Should we be using check_str_loc() in the post-image? >> >> Yes, and check_uint_loc() and check_pointer_eq_loc() as well. Which >> would be a pain. Or we drag everything into the macro check_strvec and >> get the caller's line number for free. > > Is it that big of a pain? It's mostly just passing "loc" along to the > relative functions. That part is bearable. The pain comes from the need to pass in arguments multiple times, for the stringified check description, as part of the check result and as separate values. It could be mitigated by adding a new macro that takes loc, a, op, and b, perhaps called CHECK_UINT_LOC. That additional evaluation step doesn't work nicely with arguments that are themselves macros, though, like NULL for string or pointer checks, as those will be expanded, changing the message. > Are the unit tests themselves multi-threaded within a single program? No, and check_pointer_eq, check_int, check_uint, and check_char use shared global variables to store temporary values (comments rightly warn that "this is not thread safe"). > I'd think not. In which case, I kind of wonder if a simpler pattern > would be to just set a global "location" variable (probably as a stack > of strings) that all of the individual check functions used. And then we > could set it once at the top-level of the test which wants its location > reported, and any helper functions that get called in the middle would > not have to care. And existing check_foo() functions could use the > current file/line location if the stack is empty (so code that isn't > using helper functions can remain unaffected). That doesn't sound simpler to me, quite the opposite actually. To the point that I don't dare to ask for a demo. o_O Or perhaps I need to revisit it when I'm no longer tired and hungry. René
diff --git a/t/unit-tests/t-strvec.c b/t/unit-tests/t-strvec.c index d4615ab06d..236203af61 100644 --- a/t/unit-tests/t-strvec.c +++ b/t/unit-tests/t-strvec.c @@ -17,12 +17,12 @@ static void check_strvec_loc(const char *loc, struct strvec *vec, ...) break; if (!check_uint(vec->nr, >, nr) || - !check_uint(vec->alloc, >, nr) || - !check_str(vec->v[nr], str)) { - struct strbuf msg = STRBUF_INIT; - strbuf_addf(&msg, "strvec index %"PRIuMAX, (uintmax_t) nr); - test_assert(loc, msg.buf, 0); - strbuf_release(&msg); + !check_uint(vec->alloc, >, nr)) { + va_end(ap); + return; + } + if (!check_str(vec->v[nr], str)) { + test_msg(" nr: %"PRIuMAX, (uintmax_t)nr); va_end(ap); return; }
check_strvec_loc() checks each strvec item by looping through them and comparing them with expected values. If a check fails then we'd like to know which item is affected. It reports that information by building a strbuf and delivering its contents using a failing assertion, e.g. if there are fewer items in the strvec than expected: # check "vec->nr > nr" failed at t/unit-tests/t-strvec.c:19 # left: 1 # right: 1 # check "strvec index 1" failed at t/unit-tests/t-strvec.c:71 Note that the index variable is "nr" and thus the interesting value is reported twice in that example (in lines three and four). Stop printing the index explicitly for checks that already report it. The message for the same condition as above becomes: # check "vec->nr > nr" failed at t/unit-tests/t-strvec.c:19 # left: 1 # right: 1 For the string comparison, whose error message doesn't include the index, report it using the simpler and more appropriate test_msg() instead. Report the index using its actual variable name and format the line like the preceding ones. The message for an unexpected string value becomes: # check "!strcmp(vec->v[nr], str)" failed at t/unit-tests/t-strvec.c:24 # left: "foo" # right: "bar" # nr: 0 Reported-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: René Scharfe <l.s.r@web.de> --- Changes since v1: - Typo fix. - Grammar fix. - Reworded problem description for brevity. - Qualify "name" in the last paragraph for clarity. - Add sign-off. - No code changes. Thank you, Eric! t/unit-tests/t-strvec.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) -- 2.45.2