Re: [PATCH] tests/qtest: Standardize qtest function caller strings.

[Fabiano Rosas <farosas@suse.de>]

Het Gala <het.gala@nutanix.com> writes:

> On 27/03/24 2:37 am, Fabiano Rosas wrote:
>> Het Gala<het.gala@nutanix.com>  writes:
>>
>> Some comments, mostly just thinking out loud...
>>
>>> For <test-type> --> migrate
>>> /<test-type>/<migration-mode>/<method>/<transport>/<invocation>/
>>> <compression>/<encryption>/O:<others>/...
>>>
>>> For <test-type> --> validate
>>> /<test-type>/<validate-variable>/O:<transport>/O:<invocation>/
>>> <validate-test-result>/O:<test-reason>/O:<others>/...
>> Do we need an optional 'capability' element? I'm not sure how practical
>> is to leave that as 'others', because that puts it at the end of the
>> string. We'd want the element that's more important/with more variants
>> to be towards the start of the string so we can run all tests of the
>> same kind with the -r option.
> While also looking at different functions for figuring out the transport
> and invocation, my observation was that, there might be many capabilities
> added to the same test, while it might not be important also.
> Ex: /migrate/multifd/tcp/plain
> 1. multifd is defined as a migration mode.
> 2. It is also a capability, and comes in 2 parts [multifd, multifd-channels]
>     though one is a capability and another is parameter
> Similarly in other examples of compression, there are many capabilities
> and parameters added, but it might be not important to mention that ?
>
> Secondly, there are multiple migration capabilities IIRC (> 15). And a test
> requiring multiple capabilities, the overall string would be too long, and
> not that important also to mention all capabilities.
>
> Just thinking out of mind - Can we have selective list of capabilities ? 
> 1. multifd 2. compress (again, there might be confusion with multifd 
> compression methods like zstd, zlib and just 'compress') 3. zero-page 
> (This will have sub capabilities ?)

I was thinking of keeping that part more open-ended. So not specifying
capabilities one by one, but more like "if you're testing a capability,
it comes here".

About multifd, it's a bit special since it cannot be seen as just a
"feature" anymore. It's a core part of the migration code. I wouldn't
classify it as capability for the purposes of the tests.

>
>>> test-type            :: migrate | validate
>> We could alternatively drop migration|migrate|validate. They are kind of
>> superfluous.
> I agree with the above comment. 'migrate' and 'validate' have a different
> set of variables required, some necessary, while other optional. IMO this
> will help is in streamlining the design further.
>>> migration-mode
>>>    a. migrate -->     :: precopy | postcopy | multifd
>>>    b. validate -->    :: (what to validate)
>>> methods              :: preempt | recovery | reboot | suspend | simple
> I want some inputs here.
> 1. is there a better variable name rather than 'methods'

Does this fall into the "mode" terminology that Steven introduced?

> 2. 'simple' does not fit perfect here IMO.

Can we go without it?

>>> transport            :: tcp | fd | unix | file
>>> invocation           :: uri | channels | both
>>> CompressionType      :: zlib | zstd | none
>> s/none/nocomp/ ? We're already familiar with that.
> Ack. Will change that.
>>> encryptionType       :: tls | plain
>> s/plain/notls/ ?
> What if there is another encryption technique in future ?
>> Or maybe we simply omit the noop options. It would make the string way
>> shorter in most cases.
> This might be a better approach. Can have some keys/variables as optional
> while some necessary. For ex: for 'migrate' - transport and invocation
> might be necessary while it might not be necessary for 'validate' qtests

Yep

>>> validate-test-result :: success | failure
>>> others               :: other comments/capability that needs to be
>>>                          addressed. Can be multiple
>>>
>>> (more than one applicable, separated by using '-' in between)
>>> O: optional
>>>
>>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>>> Suggested-by: Fabiano Rosas<farosas@suse.de>
>>> ---
>>>   tests/qtest/migration-test.c | 143 ++++++++++++++++++-----------------
>>>   1 file changed, 72 insertions(+), 71 deletions(-)
>>>
>>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>>> index bd9f4b9dbb..bf4d000b76 100644
>>> --- a/tests/qtest/migration-test.c
>>> +++ b/tests/qtest/migration-test.c
> Regards,
> Het Gala

I'm wondering whether we should leave the existing tests untouched and
require the new format only for new tests. Going through a git bisection
with a change in the middle that alters test names would be infuriating.