Re: [PATCH v4 18/19] tests: Add postcopy tls recovery migration test

[Peter Xu]

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,

-- 
Peter Xu