Re: [Qemu-devel] [PATCH v3 2/6] tests: migration test deprecated commands

[Juan Quintela]

Peter Xu <address@hidden> wrote:
> On Tue, Dec 26, 2017 at 08:51:10PM +0100, Juan Quintela wrote:
>> Peter Xu <address@hidden> wrote:
>> > On Fri, Dec 01, 2017 at 01:58:09PM +0100, Juan Quintela wrote:
>> >> We now test the deprecated commands everytime that we test the new
>> >> commands.  This makes unnecesary to add tests for deprecated commands.
>> >> 
>> >> Signed-off-by: Juan Quintela <address@hidden>
>> >> ---
>> >>  tests/migration-test.c | 32 ++++++++++++++++++++++++++++----
>> >>  1 file changed, 28 insertions(+), 4 deletions(-)
>> >> 
>> >> diff --git a/tests/migration-test.c b/tests/migration-test.c
>> >> index 799e24ebc6..51f49c74e9 100644
>> >> --- a/tests/migration-test.c
>> >> +++ b/tests/migration-test.c
>> >> @@ -369,7 +369,7 @@ static void migrate_check_parameter(QTestState *who, 
>> >> const char *parameter,
>> >>      QDECREF(rsp);
>> >>  }
>> >>  
>> >> -static void migrate_set_downtime(QTestState *who, const double value)
>> >> +static void deprecated_set_downtime(QTestState *who, const double value)
>> >>  {
>> >>      QDict *rsp;
>> >>      gchar *cmd;
>> >> @@ -388,7 +388,7 @@ static void migrate_set_downtime(QTestState *who, 
>> >> const double value)
>> >>      g_free(expected);
>> >>  }
>> >>  
>> >> -static void migrate_set_speed(QTestState *who, const char *value)
>> >> +static void deprecated_set_speed(QTestState *who, const char *value)
>> >>  {
>> >>      QDict *rsp;
>> >>      gchar *cmd;
>> >> @@ -402,6 +402,30 @@ static void migrate_set_speed(QTestState *who, const 
>> >> char *value)
>> >>      migrate_check_parameter(who, "max-bandwidth", value);
>> >>  }
>> >>  
>> >> +static void migrate_set_parameter(QTestState *who, const char *parameter,
>> >> +                                  const char *value)
>> >> +{
>> >> +    QDict *rsp;
>> >> +    gchar *cmd;
>> >> +
>> >> +    if (strcmp(parameter, "downtime-limit") == 0) {
>> >> +        deprecated_set_downtime(who, 0.12345);
>> >> +    }
>> >> +
>> >> +    if (strcmp(parameter, "max-bandwidth") == 0) {
>> >> +        deprecated_set_speed(who, "12345");
>> >> +    }
>> >
>> > I'm fine with current approach, but I would really prefer to put them
>> > all into a standalone test, by just calling them one by one with some
>> > specific numbers and that's all.
>> 
>> That means another test (at least), and we have, also at least three
>> deprecated comands:
>> - migrate_set_speed
>> - migrate_set_downtime
>> - migrate_set_cache_size
>> 
>> And each test makes things slower.  So I *thought* it would we wiser to
>> just check _always_ use the deprecated an the not deprecated one.
>> 
>> > (luckily we only have two deprecated commands and limited tests,
>> >  otherwise extra commands will be M*N, say "number of deprecated
>> >  command" * "number of test mirations")
>> 
>> Each test takes time, so adding tests make everything much slower.
>> Notice that setting a new setting is fast.
>> 
>> This was the way that I understood Dave he wanted.
>
> Do you mean every test is slow, or just migration tests?

Each migration test adds around 2 seconds on my machine.  So I decided
that it was easier that each time that we check one command, we test the
deprecated and non-deprecated versions of the command.  Amount of time
added to the test is negigible, and we are sure that we always test both
functions.

If we ever remove the deprecated method, we can always remove only that
part of th etest.


> Here I mean
> to only test setting the parameters without doing real migration tests
> (then query to make sure the settings were taking effect).  I assume
> that should be fast too?  Thanks,

We could create a new test for that, but we need to start in on
source/destination, I thought it just made things more complicated.

If your preffer that way, please suggest how/what?

Thanks, Juan.