Re: [PATCH 0/3] tests/acceptance: Handle tests with "cpu" tag

[Eduardo Habkost <ehabkost@redhat.com>]

On Tue, Mar 23, 2021 at 05:01:09PM -0400, John Snow wrote:
> On 3/17/21 3:16 PM, Wainer dos Santos Moschetta wrote:
> > Added John and Eduardo,
> > 
> > On 3/9/21 3:52 PM, Cleber Rosa wrote:
> > > On Wed, Feb 24, 2021 at 06:26:51PM -0300, Wainer dos Santos
> > > Moschetta wrote:
> > > > Currently the acceptance tests tagged with "machine" have the "-M TYPE"
> > > > automatically added to the list of arguments of the QEMUMachine object.
> > > > In other words, that option is passed to the launched QEMU. On this
> > > > series it is implemented the same feature but instead for tests marked
> > > > with "cpu".
> > > > 
> > > Good!
> > > 
> > > > There is a caveat, however, in case the test needs additional
> > > > arguments to
> > > > the CPU type they cannot be passed via tag, because the tags
> > > > parser split
> > > > values by comma. For example, in
> > > > tests/acceptance/x86_cpu_model_versions.py,
> > > > there are cases where:
> > > > 
> > > >    * -cpu is set to
> > > > "Cascadelake-Server,x-force-features=on,check=off,enforce=off"
> > > >    * if it was tagged like
> > > > "cpu:Cascadelake-Server,x-force-features=on,check=off,enforce=off"
> > > >      then the parser would break it into 4 tags
> > > > ("cpu:Cascadelake-Server",
> > > >      "x-force-features=on", "check=off", "enforce=off")
> > > >    * resulting on "-cpu Cascadelake-Server" and the remaining
> > > > arguments are ignored.
> > > > 
> > > > For the example above, one should tag it (or not at all) as
> > > > "cpu:Cascadelake-Server"
> > > > AND self.vm.add_args('-cpu',
> > > > "Cascadelake-Server,x-force-features=on,check=off,enforce=off"),
> > > > and that results on something like:
> > > > 
> > > >    "qemu-system-x86_64 (...) -cpu Cascadelake-Server -cpu
> > > > Cascadelake-Server,x-force-features=on,check=off,enforce=off".
> > > > 
> > > There are clearly two problems here:
> > > 
> > > 1) the tag is meant to be succinct, so that it can be used by users
> > >     selecting which tests to run.  At the same time, it's a waste
> > >     to throw away the other information or keep it duplicate or
> > >     incosistent.
> > > 
> > > 2) QEMUMachine doesn't keep track of command line arguments
> > >     (add_args() makes it pretty clear what's doing).  But, on this type
> > >     of use case, a "set_args()" is desirable, in which case it would
> > >     overwrite the existing arguments for a given command line option.
> > 
> > I like the idea of a "set_args()" to QEMUMachine as you describe above
> > but it needs further discussion because I can see at least one corner
> > case; for example, one can set the machine type as either -machine or
> > -M, then what key it should be searched-and-replaced (if any) on the
> > list of args?
> > 
> > Unlike your suggestion, I thought on implement the method to deal with a
> > single argument at time, as:
> > 
> >      def set_arg(self, arg: Union[str, list], value: str) -> None:
> >          """
> >          Set the value of an argument from the list of extra arguments
> > to be
> >          given to the QEMU binary. If the argument does not exist then
> > it is
> >          added to the list.
> > 
> >          If the ``arg`` parameter is a list then it will search and
> > replace all
> >          occurencies (if any). Otherwise a new argument is added and it is
> >          used the first value of the ``arg`` list.
> >          """
> >          pass
> > 
> > Does it sound good to you?
> > 
> > Thanks!
> > 
> > Wainer
> > 
> 
> A little hokey, but I suppose that's true of our CLI interface in general.
> 
> I'd prefer not get into the business of building a "config" inside the
> python module if we can help it right now, but if "setting" individual args
> is something you truly need to do, I won't stand in the way.
> 
> Do what's least-gross.

I don't have any specific suggestions on how the API should look
like, but I'm having trouble understanding the documentation
above.

I don't know what "it will search and replace all occurrences"
means.  Occurrences of what?

I don't understand what "it is used the first value of the `arg`
list" means, either.  I understand you are going to use the first
value of the list, but you don't say what you are going to do
with it.

-- 
Eduardo