[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/3] tests: Avoid side effects inside g_assert() arguments
From: |
Peter Maydell |
Subject: |
Re: [PATCH 3/3] tests: Avoid side effects inside g_assert() arguments |
Date: |
Tue, 4 May 2021 09:46:02 +0100 |
On Tue, 4 May 2021 at 08:18, Thomas Huth <thuth@redhat.com> wrote:
>
> On 03/05/2021 18.55, Peter Maydell wrote:
> > For us, assertions are always enabled, but side-effect expressions
> > inside the argument to g_assert() are bad style anyway. Fix three
> > occurrences in IPMI related tests, which will silence some Coverity
> > nits.
> >
> > Fixes: CID 1432322, CID 1432287, CID 1432291
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > diff --git a/tests/qtest/ipmi-kcs-test.c b/tests/qtest/ipmi-kcs-test.c
> > index fc0a918c8d1..afc24dd3e46 100644
> > --- a/tests/qtest/ipmi-kcs-test.c
> > +++ b/tests/qtest/ipmi-kcs-test.c
> > @@ -73,7 +73,8 @@ static void kcs_wait_ibf(void)
> > {
> > unsigned int count = 1000;
> > while (IPMI_KCS_CMDREG_GET_IBF() != 0) {
> > - g_assert(--count != 0);
> > + --count;
> > + g_assert(count != 0);
> > }
> > }
>
> According to
> https://developer.gnome.org/glib/unstable/glib-Testing.html#g-assert
> g_assert() should be avoided in unit tests and g_assert_true() should be
> used instead. So I think it might be nicer to use g_assert_true() here?
That probably depends on what we decide about whether we want to
use glib-testing-style "assert but this might not stop execution" in our
tests: see this thread:
CAFEAcA9juOChqrh5orybJQwpQsyEZ5z3Dvmy7fjX0DW4Nbgmrg@mail.gmail.com/">https://lore.kernel.org/qemu-devel/CAFEAcA9juOChqrh5orybJQwpQsyEZ5z3Dvmy7fjX0DW4Nbgmrg@mail.gmail.com/
(I should have cc'd you and Laurent on that as qtest maintainers; sorry.)
-- PMM