| Message ID | 20211115145900.2531865-9-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | tests/unit: Fix test-smp-parse | expand |
On 11/15/21 3:58 PM, 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(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Hi Philippe, 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. int main(int argc, char *argv[]) { module_call_init(MODULE_INIT_QOM); g_test_init(&argc, &argv, NULL); g_test_add_data_func("/test-smp-parse/generic/valid", 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"), test_generic_invalid); g_test_add_data_func("/test-smp-parse/with_dies", MACHINE_TYPE_NAME("smp-with-dies"), test_with_dies); g_test_run(); return 0; } Otherwise, #7 and #8 look nice. Thanks for your work! Thanks, Yanan
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, I wanted to 1/ fix our Windows CI and 2/ show you how to structure the tests. Regards, Phil.
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. > > .
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"),
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(-)