Re: [PATCH v19 7/8] tests/numa: Add case for QMP build HMAT

[Tao Xu]

On 11/28/2019 7:53 PM, Thomas Huth wrote:
> On 28/11/2019 12.49, Markus Armbruster wrote:
> > Tao Xu <address@hidden> writes:
> >
> > > Check configuring HMAT usecase
> > >
> > > Reviewed-by: Igor Mammedov <address@hidden>
> > > Suggested-by: Igor Mammedov <address@hidden>
> > > Signed-off-by: Tao Xu <address@hidden>
> > > ---
> > >
> > > Changes in v19:
> > >       - Add some fail cases for hmat-cache when level=0
> > >
> > > Changes in v18:
> > >       - Rewrite the lines over 80 characters
> > >
> > > Chenges in v17:
> > >       - Add some fail test cases (Igor)
> > > ---
> > >    tests/numa-test.c | 213 ++++++++++++++++++++++++++++++++++++++++++++++
> > >    1 file changed, 213 insertions(+)
> > >
> > > diff --git a/tests/numa-test.c b/tests/numa-test.c
> > > index 8de8581231..aed7b2f31b 100644
> > > --- a/tests/numa-test.c
> > > +++ b/tests/numa-test.c
> > > @@ -327,6 +327,216 @@ static void pc_dynamic_cpu_cfg(const void *data)
> > >        qtest_quit(qs);
> > >    }
> > >    
> > > +static void pc_hmat_build_cfg(const void *data)
> > > +{
> > > +    QTestState *qs = qtest_initf("%s -nodefaults --preconfig -machine hmat=on "
> > > +                     "-smp 2,sockets=2 "
> > > +                     "-m 128M,slots=2,maxmem=1G "
> > > +                     "-object memory-backend-ram,size=64M,id=m0 "
> > > +                     "-object memory-backend-ram,size=64M,id=m1 "
> > > +                     "-numa node,nodeid=0,memdev=m0 "
> > > +                     "-numa node,nodeid=1,memdev=m1,initiator=0 "
> > > +                     "-numa cpu,node-id=0,socket-id=0 "
> > > +                     "-numa cpu,node-id=0,socket-id=1",
> > > +                     data ? (char *)data : "");
> > > +
> > > +    /* Fail: Initiator should be less than the number of nodes */
> > > +    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
> > > +        " 'arguments': { 'type': 'hmat-lb', 'initiator': 2, 'target': 0,"
> > > +        " 'hierarchy': \"memory\", 'data-type': \"access-latency\" } }")));
> >
> > Code smell: side effect within assert().
> >
> > Harmless here, because compiling tests with NDEBUG is pointless.  Still,
> > it sets a bad example.  Not your idea, the pattern seems to go back to
> > commit c35665e1ee3 and fb1e58f72ba.
>
> ... maybe best to use g_assert_true() which can't be disabled and thus
> should be used in tests. See:
>
>    https://developer.gnome.org/glib/unstable/glib-Testing.html#g-assert-true
>
>    Thomas
Thank you for your suggestion. I will use g_assert_true and 
g_assert_false to replace g_assert