Re: [PATCH-for-7.0 v4 08/11] tests/unit/test-smp-parse: Add 'smp-without-dies-valid' machine type

[wangyanan (Y)]

On 2021/11/17 16:08, Philippe Mathieu-Daudé wrote:
> Hi Yanan,
>
> On 11/17/21 08:37, wangyanan (Y) wrote:
> > On 2021/11/15 22:58, Philippe Mathieu-Daudé wrote:
> > > Keep the common TYPE_MACHINE class initialization in
> > > machine_base_class_init(), make it abstract, and move
> > > the non-common code to a new class: "smp-without-dies-valid".
> > >
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > ---
> > >    tests/unit/test-smp-parse.c | 19 +++++++++++++++----
> > >    1 file changed, 15 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
> > > index dfe7f1313b0..90a249fe8c8 100644
> > > --- a/tests/unit/test-smp-parse.c
> > > +++ b/tests/unit/test-smp-parse.c
> > > @@ -478,13 +478,19 @@ static void machine_base_class_init(ObjectClass
> > > *oc, void *data)
> > >    {
> > >        MachineClass *mc = MACHINE_CLASS(oc);
> > >    +    mc->smp_props.prefer_sockets = true;
> > > +
> > > +    mc->name = g_strdup(SMP_MACHINE_NAME);
> > > +}
> > > +
> > > +static void machine_without_dies_valid_class_init(ObjectClass *oc,
> > > void *data)
> > > +{
> > > +    MachineClass *mc = MACHINE_CLASS(oc);
> > > +
> > >        mc->min_cpus = MIN_CPUS;
> > >        mc->max_cpus = MAX_CPUS;
> > >    -    mc->smp_props.prefer_sockets = true;
> > >        mc->smp_props.dies_supported = false;
> > > -
> > > -    mc->name = g_strdup(SMP_MACHINE_NAME);
> > >    }
> > >      static void machine_without_dies_invalid_class_init(ObjectClass
> > > *oc, void *data)
> > > @@ -606,9 +612,14 @@ static const TypeInfo smp_machine_types[] = {
> > >        {
> > >            .name           = TYPE_MACHINE,
> > >            .parent         = TYPE_OBJECT,
> > > +        .abstract       = true,
> > >            .class_init     = machine_base_class_init,
> > >            .class_size     = sizeof(MachineClass),
> > >            .instance_size  = sizeof(MachineState),
> > > +    }, {
> > > +        .name           = MACHINE_TYPE_NAME("smp-without-dies-valid"),
> > > +        .parent         = TYPE_MACHINE,
> > > +        .class_init     = machine_without_dies_valid_class_init,
> > >        }, {
> > >            .name           =
> > > MACHINE_TYPE_NAME("smp-without-dies-invalid"),
> > >            .parent         = TYPE_MACHINE,
> > > @@ -629,7 +640,7 @@ int main(int argc, char *argv[])
> > >        g_test_init(&argc, &argv, NULL);
> > >          g_test_add_data_func("/test-smp-parse/generic/valid",
> > > -                         TYPE_MACHINE,
> > > +                         MACHINE_TYPE_NAME("smp-without-dies-valid"),
> > >                             test_generic_valid);
> > >        g_test_add_data_func("/test-smp-parse/generic/invalid",
> > >                             MACHINE_TYPE_NAME("smp-without-dies-invalid"),
> > After patch #7 and #8, we will have sub-tests as below. It looks nice,
> > but it will
> > probably be better to tweak "smp-without-dies-valid" to
> > "smp-generic-valid",
> > and "smp-without-dies-invalid" to "smp-generic-invalid", which will be more
> > consistent with the corresponding sub-test name.
> >
> > It's Ok now as we only have dies currently besides generic
> > sockets/cores/threads,
> > but "smp-without-dies-xxx" will become a bit confusing when new topology
> > members are introduced and tested here.
> OK I modified it and will respin once v6.2 is released.
>
> Also test_with_dies() should be split in 2 tests: valid/invalid;
> then smp_parse_test() should split tests further regarding the
> socket preference. But I'll let that to you,
Sure, I can do this in an appropriate time. But IMHO, I don't see a
strong necessity to split test_with_dies for now, as the valid/invalid
scenarios share the same class properties. We can probably keep it
as is until we have to split it.

As for socket preference, I can foresee code duplication if we split
all the tests into two parts just regarding the socket preference.
Isn't it clear enough to use current smp_parse_test() for each
test data sample? Do we have other concern here?
> I wanted to 1/ fix
> our Windows CI and 2/ show you how to structure the tests.
Understood. The test architecture is indeed improved a lot.
Thanks for your education. 😉

Thanks,
Yanan
> Regards,
>
> Phil.
>
> .