| Message ID | 20220331150857.74406-19-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Postcopy Preemption | expand |
On Thu, Mar 31, 2022 at 11:08:56AM -0400, Peter Xu wrote: > It's easy to build this upon the postcopy tls test. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > tests/qtest/migration-test.c | 27 +++++++++++++++++++++------ > 1 file changed, 21 insertions(+), 6 deletions(-) > > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c > index 80c4244871..7288c64e97 100644 > --- a/tests/qtest/migration-test.c > +++ b/tests/qtest/migration-test.c > @@ -1058,15 +1058,15 @@ static void test_postcopy_tls(void) > test_postcopy_common(&args); > } > > -static void test_postcopy_recovery(void) > +static void test_postcopy_recovery_common(MigrateStart *args) > { > - MigrateStart args = { > - .hide_stderr = true, > - }; > QTestState *from, *to; > g_autofree char *uri = NULL; > > - if (migrate_postcopy_prepare(&from, &to, &args)) { > + /* Always hide errors for postcopy recover tests since they're expected */ > + args->hide_stderr = true; > + > + if (migrate_postcopy_prepare(&from, &to, args)) { > return; > } > > @@ -1117,7 +1117,21 @@ static void test_postcopy_recovery(void) > /* Restore the postcopy bandwidth to unlimited */ > migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0); > > - migrate_postcopy_complete(from, to, &args); > + migrate_postcopy_complete(from, to, args); > +} > + > +static void test_postcopy_recovery(void) > +{ > + MigrateStart args = { }; > + > + test_postcopy_recovery_common(&args); > +} > + > +static void test_postcopy_recovery_tls(void) > +{ > + MigrateStart args = { .postcopy_tls = true }; > + > + test_postcopy_recovery_common(&args); > } > > static void test_baddest(void) > @@ -2164,6 +2178,7 @@ int main(int argc, char **argv) > qtest_add_func("/migration/postcopy/recovery", test_postcopy_recovery); > #ifdef CONFIG_GNUTLS > qtest_add_func("/migration/postcopy/tls", test_postcopy_tls); > + qtest_add_func("/migration/postcopy/tls/recovery", test_postcopy_recovery_tls); It is important that a test name is *NOT* a prefix for another test name, as that makes it harder to selectively run individual tests with '-p' as it does a pattern match. Bearing in mind my comments on the previous patch, I think we want /migration/postcopy/recovery/plain /migration/postcopy/recovery/tls/psk With regards, Daniel
On Wed, Apr 20, 2022 at 12:42:15PM +0100, Daniel P. Berrangé wrote: > On Thu, Mar 31, 2022 at 11:08:56AM -0400, Peter Xu wrote: > > It's easy to build this upon the postcopy tls test. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > tests/qtest/migration-test.c | 27 +++++++++++++++++++++------ > > 1 file changed, 21 insertions(+), 6 deletions(-) > > > > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c > > index 80c4244871..7288c64e97 100644 > > --- a/tests/qtest/migration-test.c > > +++ b/tests/qtest/migration-test.c > > @@ -1058,15 +1058,15 @@ static void test_postcopy_tls(void) > > test_postcopy_common(&args); > > } > > > > -static void test_postcopy_recovery(void) > > +static void test_postcopy_recovery_common(MigrateStart *args) > > { > > - MigrateStart args = { > > - .hide_stderr = true, > > - }; > > QTestState *from, *to; > > g_autofree char *uri = NULL; > > > > - if (migrate_postcopy_prepare(&from, &to, &args)) { > > + /* Always hide errors for postcopy recover tests since they're expected */ > > + args->hide_stderr = true; > > + > > + if (migrate_postcopy_prepare(&from, &to, args)) { > > return; > > } > > > > @@ -1117,7 +1117,21 @@ static void test_postcopy_recovery(void) > > /* Restore the postcopy bandwidth to unlimited */ > > migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0); > > > > - migrate_postcopy_complete(from, to, &args); > > + migrate_postcopy_complete(from, to, args); > > +} > > + > > +static void test_postcopy_recovery(void) > > +{ > > + MigrateStart args = { }; > > + > > + test_postcopy_recovery_common(&args); > > +} > > + > > +static void test_postcopy_recovery_tls(void) > > +{ > > + MigrateStart args = { .postcopy_tls = true }; > > + > > + test_postcopy_recovery_common(&args); > > } > > > > static void test_baddest(void) > > @@ -2164,6 +2178,7 @@ int main(int argc, char **argv) > > qtest_add_func("/migration/postcopy/recovery", test_postcopy_recovery); > > #ifdef CONFIG_GNUTLS > > qtest_add_func("/migration/postcopy/tls", test_postcopy_tls); > > + qtest_add_func("/migration/postcopy/tls/recovery", test_postcopy_recovery_tls); > > It is important that a test name is *NOT* a prefix for another > test name, as that makes it harder to selectively run individual > tests with '-p' as it does a pattern match. > > Bearing in mind my comments on the previous patch, I think we want > > /migration/postcopy/recovery/plain > /migration/postcopy/recovery/tls/psk Again, I can try to take all the suggestions in the next version, but note that there's no obvious reason on how we name them.. It's: /XXX/Feature1 /XXX/Feature2 ... Now what we're saying is: /XXX/Feature1/Feature2 is better than /XXX/Feature2/Feature1. And IMHO that really does not matter.. To be strict, for features that are compatible between each other, the only sane way to write them is: /XXX/Feature1 /XXX/Feature2 /XXX/Feature1+Feature2 And we make sure there's an ordered list of features. But then we still lose the ultimate goal of allowing us to specify one "-p something" to run any tests that FeatureX is enabled. Sometimes we simply run a superset or subset then it's good enough at least to me. IOW, we may need something better than the path-form (-p) of qtest to achieve what you wanted, IMHO. Thanks,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index 80c4244871..7288c64e97 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c @@ -1058,15 +1058,15 @@ static void test_postcopy_tls(void) test_postcopy_common(&args); } -static void test_postcopy_recovery(void) +static void test_postcopy_recovery_common(MigrateStart *args) { - MigrateStart args = { - .hide_stderr = true, - }; QTestState *from, *to; g_autofree char *uri = NULL; - if (migrate_postcopy_prepare(&from, &to, &args)) { + /* Always hide errors for postcopy recover tests since they're expected */ + args->hide_stderr = true; + + if (migrate_postcopy_prepare(&from, &to, args)) { return; } @@ -1117,7 +1117,21 @@ static void test_postcopy_recovery(void) /* Restore the postcopy bandwidth to unlimited */ migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0); - migrate_postcopy_complete(from, to, &args); + migrate_postcopy_complete(from, to, args); +} + +static void test_postcopy_recovery(void) +{ + MigrateStart args = { }; + + test_postcopy_recovery_common(&args); +} + +static void test_postcopy_recovery_tls(void) +{ + MigrateStart args = { .postcopy_tls = true }; + + test_postcopy_recovery_common(&args); } static void test_baddest(void) @@ -2164,6 +2178,7 @@ int main(int argc, char **argv) qtest_add_func("/migration/postcopy/recovery", test_postcopy_recovery); #ifdef CONFIG_GNUTLS qtest_add_func("/migration/postcopy/tls", test_postcopy_tls); + qtest_add_func("/migration/postcopy/tls/recovery", test_postcopy_recovery_tls); #endif /* CONFIG_GNUTLS */ qtest_add_func("/migration/bad_dest", test_baddest); qtest_add_func("/migration/precopy/unix/plain", test_precopy_unix_plain);
It's easy to build this upon the postcopy tls test. Signed-off-by: Peter Xu <peterx@redhat.com> --- tests/qtest/migration-test.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-)