Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"

[Peter Xu]

On Fri, Apr 12, 2024 at 11:58:43AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Thu, Apr 11, 2024 at 04:41:16PM -0300, Fabiano Rosas wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > On Thu, Apr 11, 2024 at 11:31:08PM +0530, Het Gala wrote:
> >> >> I just wanted to highlight couple of pointers:
> >> >> 1. though we are using 'channels' in the precopy tests for 'migrate' 
> >> >> QAPI,
> >> >> we
> >> >>    use the old uri for 'migrate-incoming' QAPI.
> >> >> 2. We do not cover other 'channels' abi, only have tcp path tested.
> >> >> 
> >> >> So, the TO-DOs could be:
> >> >> 1. Omit the 4th patch here, which introduced postcopy qtests with 
> >> >> 'channels'
> >> >>    interface OR have 'channels' interface with other than tcp transport
> >> >>    (file, exec, vsock, etc) so as to cover different code paths.
> >> >> 2. Extend channels interface to migrate-incoming QAPI for precopy qtests
> >> >
> >> > You can see whether Fabiano has anything to say, but what you proposed
> >> > looks good to me.
> >> 
> >> Ok, so what about we convert some of the 'plain' tests into channels to
> >> cover all transports?
> >> 
> >> - tcp: test_multifd_tcp_none  (this one we already did)
> >> - file: test_precopy_file
> >> - unix: test_precopy_unix_plain
> >> - exec: test_analyze_script
> >> - fd: test_migrate_precopy_fd_socket
> >> 
> >> Those^, plus the validate_uri that's already in next should cover
> >> everything.
> >> 
> >> We don't need to do this at once, by the way.
> >> 
> >> Moreover:
> >> 
> >> - leave all test strings untouched to preserve bisecting;
> >> 
> >> - let's not bother adding "channels" and "uri" to the test string
> >>   anymore. The channels API should be taken for granted at this point, I
> >>   don't expect we start hitting bugs that will require us to run either
> >>   foo/uri/plain or foo/channels/plain, so there's not much point in
> >>   making the distinction.
> >
> > Do you mean we can put "uri:" aside?  Maybe I misunderstood..
> 
> I mean the test name does not need to specify "channels" vs. "uri"
> because that should never be broken to the point that we actually need
> to go fetch those tests by name. We'd still have at least 1 test for
> each transport with channels and (existing) at least 1 test for each
> transport with uri.
> 
> >
> > The matrix previously was (I think.. when this series posted):
> >
> >   [tcp, unix, file, exec, fd] x [uri, channels] x [precopy, postcopy]
> >
> > Drop postcopy as doesn't seem to have any special paths:
> >
> >   [tcp, unix, file, exec, fd] x [uri, channels]
> >
> > So logically we should still cover these, right?
> 
> Right, I'm just suggesting we convert some tests to use channels, one
> for each transport, to test the channels API in full. The rest of the
> existing tests as well as future tests need not have a uri (or channel)
> variant. Just one of them is enough.

Ah so that's the "test string"; sounds all good.

-- 
Peter Xu