In order to allow out-of-tree acceptance tests with Avocado,
create a symlink in the build tree.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Based-on: 20180530184156.15634-1-crosa@redhat.com
http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg06877.html

Before:

  build_dir$ avocado run /full/path/to/sources/qemu/tests/acceptance/boot_linux_console.py

After

  build_dir$ avocado run tests/acceptance/boot_linux_console.py

I first wanted to try something like:

echo "[datadir.paths]" > .avocado.conf
echo "test_dir = $source_path/tests/acceptance" >> .avocado.conf

to run:

  build_dir$ avocado run -t console

but this doesn't work this way, I'd have to use

  build_dir$ avocado --config .avocado.conf boot_linux_console.py -t console

which isn't a win.
---
 configure | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index a6a4616c3e..0763ebe5ba 100755
--- a/configure
+++ b/configure
@@ -7213,9 +7213,11 @@ for rom in seabios vgabios ; do
 done
 
 # set up tests data directory
-if [ ! -e tests/data ]; then
-    symlink "$source_path/tests/data" tests/data
-fi
+for tests_subdir in acceptance data; do
+    if [ ! -e tests/$tests_subdir ]; then
+        symlink "$source_path/tests/$tests_subdir" tests/$tests_subdir
+    fi
+done
 
 # set up qemu-iotests in this build directory
 iotests_common_env="tests/qemu-iotests/common.env"
-- 
2.17.1

On Wed, May 30, 2018 at 05:03:34PM -0300, Philippe Mathieu-Daudé wrote:
> In order to allow out-of-tree acceptance tests with Avocado,
> create a symlink in the build tree.
> 

"In other to allow" seems to imply that running acceptance tests
on an out-of-tree build is impossible without this patch, but:

[...]
> Before:
> 
>   build_dir$ avocado run /full/path/to/sources/qemu/tests/acceptance/boot_linux_console.py
> 
> After
> 
>   build_dir$ avocado run tests/acceptance/boot_linux_console.py
> 

This seems to imply that running them is possible, but this patch
makes it more convenient.

Is this just for convenience, or necessary to allow acceptance
tests to run properly?  Can this be clarified in the commit
message?


> I first wanted to try something like:
> 
> echo "[datadir.paths]" > .avocado.conf
> echo "test_dir = $source_path/tests/acceptance" >> .avocado.conf
> 
> to run:
> 
>   build_dir$ avocado run -t console
> 
> but this doesn't work this way, I'd have to use
> 
>   build_dir$ avocado --config .avocado.conf boot_linux_console.py -t console
> 
> which isn't a win.
> ---
>  configure | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/configure b/configure
> index a6a4616c3e..0763ebe5ba 100755
> --- a/configure
> +++ b/configure
> @@ -7213,9 +7213,11 @@ for rom in seabios vgabios ; do
>  done
>  
>  # set up tests data directory
> -if [ ! -e tests/data ]; then
> -    symlink "$source_path/tests/data" tests/data
> -fi
> +for tests_subdir in acceptance data; do
> +    if [ ! -e tests/$tests_subdir ]; then
> +        symlink "$source_path/tests/$tests_subdir" tests/$tests_subdir
> +    fi
> +done
>  
>  # set up qemu-iotests in this build directory
>  iotests_common_env="tests/qemu-iotests/common.env"
> -- 
> 2.17.1
> 

-- 
Eduardo

On 05/30/2018 04:03 PM, Philippe Mathieu-Daudé wrote:
> In order to allow out-of-tree acceptance tests with Avocado,
> create a symlink in the build tree.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Based-on: 20180530184156.15634-1-crosa@redhat.com
> http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg06877.html
> 
> Before:
> 
>   build_dir$ avocado run /full/path/to/sources/qemu/tests/acceptance/boot_linux_console.py
> 
> After
> 
>   build_dir$ avocado run tests/acceptance/boot_linux_console.py
> 
> I first wanted to try something like:
> 
> echo "[datadir.paths]" > .avocado.conf
> echo "test_dir = $source_path/tests/acceptance" >> .avocado.conf
> 

FIY, "$HOME/.config/avocado/avocado.conf" is parsed by default.  Take a
look at the "avocado config" output for the details.

> to run:
> 
>   build_dir$ avocado run -t console
> 

I see your point, you'd like the command above to behave similarly to:

  avocado run -t console $avocado_datadir_paths_test_dir

Right?  So, the first compromise solution with current Avocado is:

  echo "[datadir.paths]" > ~/.config/avocado/avocado.conf
  echo "test_dir = $source_path/tests/acceptance" >>
~/.config/avocado/avocado.conf

With "$source_path" being something literal, and not a variable.  It
allows individual tests to be found at that location:

  cd /some/random/path
  avocado run boot_linux_console.py

But the following still won't work:

  avocado run -t console
  No test references provided nor any other arguments resolved into
tests. Please double check the executed command.

I do think the current Avocado behavior is somewhat inconsistent,
because list and run should ideally be symmetrical.  With the
configuration above, "list" behaves like this:

   avocado list
INSTRUMENTED
/home/cleber/src/qemu/tests/acceptance/boot_linux_console.py:BootLinuxConsole.test
INSTRUMENTED
/home/cleber/src/qemu/tests/acceptance/version.py:Version.test_qmp_human_info_version
INSTRUMENTED /home/cleber/src/qemu/tests/acceptance/vnc.py:Vnc.test_no_vnc
INSTRUMENTED
/home/cleber/src/qemu/tests/acceptance/vnc.py:Vnc.test_no_vnc_change_password
INSTRUMENTED
/home/cleber/src/qemu/tests/acceptance/vnc.py:Vnc.test_vnc_change_password_requires_a_password
INSTRUMENTED
/home/cleber/src/qemu/tests/acceptance/vnc.py:Vnc.test_vnc_change_password

But at the same, there are security implications: `list` won't
load/execute any test code (different from, say, standard Python
unittests), while "run" obviously will.  So "avocado run" may end up
running what users don't want if a malicious user controls
"$avocado_datadir_paths_test_dir".

What if Avocado introduces an option that would enable the use of
Avocado's test dir when no test reference is given?  This could be
enabled only once (but would be disabled by default):

  echo "[loader]" > ~/.config/avocado/avocado.conf
  echo "use_test_dir_when_no_references_given = True" >> >
~/.config/avocado/avocado.conf

  echo "[datadir.paths]" >> ~/.config/avocado/avocado.conf
  echo "test_dir = $source_path/tests/acceptance" >>
~/.config/avocado/avocado.conf

And after that, the following would run all "console" tests:

  avocado run -t console

How does this sound?

- Cleber.

> but this doesn't work this way, I'd have to use
> 
>   build_dir$ avocado --config .avocado.conf boot_linux_console.py -t console
> 
> which isn't a win.
> ---
>  configure | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/configure b/configure
> index a6a4616c3e..0763ebe5ba 100755
> --- a/configure
> +++ b/configure
> @@ -7213,9 +7213,11 @@ for rom in seabios vgabios ; do
>  done
>  
>  # set up tests data directory
> -if [ ! -e tests/data ]; then
> -    symlink "$source_path/tests/data" tests/data
> -fi
> +for tests_subdir in acceptance data; do
> +    if [ ! -e tests/$tests_subdir ]; then
> +        symlink "$source_path/tests/$tests_subdir" tests/$tests_subdir
> +    fi
> +done
>  
>  # set up qemu-iotests in this build directory
>  iotests_common_env="tests/qemu-iotests/common.env"
> 

On 05/30/2018 10:06 PM, Cleber Rosa wrote:
> On 05/30/2018 04:03 PM, Philippe Mathieu-Daudé wrote:
>> In order to allow out-of-tree acceptance tests with Avocado,
>> create a symlink in the build tree.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> Based-on: 20180530184156.15634-1-crosa@redhat.com
>> http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg06877.html
>>
>> Before:
>>
>>   build_dir$ avocado run /full/path/to/sources/qemu/tests/acceptance/boot_linux_console.py
>>
>> After
>>
>>   build_dir$ avocado run tests/acceptance/boot_linux_console.py
>>
>> I first wanted to try something like:
>>
>> echo "[datadir.paths]" > .avocado.conf
>> echo "test_dir = $source_path/tests/acceptance" >> .avocado.conf
>>
> 
> FIY, "$HOME/.config/avocado/avocado.conf" is parsed by default.  Take a
> look at the "avocado config" output for the details.

I followed the virtualenv installation from the link you added in
testing.rst:
http://avocado-framework.readthedocs.io/en/latest/GetStartedGuide.html#installing-avocado

So my config is:

Config files read (in order):

/builds/qemu/venv2/local/lib/python2.7/site-packages/avocado/etc/avocado/avocado.conf

/builds/qemu/venv2/local/lib/python2.7/site-packages/avocado/etc/avocado/conf.d/result_upload.conf

/builds/qemu/venv2/local/lib/python2.7/site-packages/avocado/etc/avocado/conf.d/resultsdb.conf

/builds/qemu/venv2/local/lib/python2.7/site-packages/avocado/etc/avocado/conf.d/jobscripts.conf

/builds/qemu/venv2/local/lib/python2.7/site-packages/avocado/etc/avocado/conf.d/gdb.conf

    Section.Key                              Value
    datadir.paths.base_dir                   /var/lib/avocado
    datadir.paths.test_dir                   /usr/share/avocado/tests
    datadir.paths.data_dir                   /var/lib/avocado/data
    datadir.paths.logs_dir                   ~/avocado/job-results

I find useful to be able to append/overwrite the user 'global' config
with a current directory one, like we have with .gdbinit, because I use
out-of-tree build dir to test different build options/features with the
same codebase.

So personally I'd like this parse order:

- $VENV/lib/python2.7/site-packages/avocado/etc/avocado/avocado.conf
- $HOME/.config/avocado/avocado.conf
- .avocado.conf

> 
>> to run:
>>
>>   build_dir$ avocado run -t console
>>
> 
> I see your point, you'd like the command above to behave similarly to:
> 
>   avocado run -t console $avocado_datadir_paths_test_dir
> 
> Right?  So, the first compromise solution with current Avocado is:
> 
>   echo "[datadir.paths]" > ~/.config/avocado/avocado.conf
>   echo "test_dir = $source_path/tests/acceptance" >>
> ~/.config/avocado/avocado.conf
> 
> With "$source_path" being something literal, and not a variable.  It
> allows individual tests to be found at that location:
> 
>   cd /some/random/path
>   avocado run boot_linux_console.py

'echo' is not that friendly :S

$ avocado run boot_linux_console.py
Avocado crashed unexpectedly: Value 'logs_dir' not found in section
'datadir.paths'
You can find details in
/var/tmp/avocado-traceback-2018-06-05_11:32:01-MUn_kE.log

> 
> But the following still won't work:
> 
>   avocado run -t console
>   No test references provided nor any other arguments resolved into
> tests. Please double check the executed command.
> 
> I do think the current Avocado behavior is somewhat inconsistent,
> because list and run should ideally be symmetrical.  With the
> configuration above, "list" behaves like this:
> 
>    avocado list
> INSTRUMENTED
> /home/cleber/src/qemu/tests/acceptance/boot_linux_console.py:BootLinuxConsole.test
> INSTRUMENTED
> /home/cleber/src/qemu/tests/acceptance/version.py:Version.test_qmp_human_info_version
> INSTRUMENTED /home/cleber/src/qemu/tests/acceptance/vnc.py:Vnc.test_no_vnc
> INSTRUMENTED
> /home/cleber/src/qemu/tests/acceptance/vnc.py:Vnc.test_no_vnc_change_password
> INSTRUMENTED
> /home/cleber/src/qemu/tests/acceptance/vnc.py:Vnc.test_vnc_change_password_requires_a_password
> INSTRUMENTED
> /home/cleber/src/qemu/tests/acceptance/vnc.py:Vnc.test_vnc_change_password
> 
> But at the same, there are security implications: `list` won't
> load/execute any test code (different from, say, standard Python
> unittests), while "run" obviously will.  So "avocado run" may end up
> running what users don't want if a malicious user controls
> "$avocado_datadir_paths_test_dir".
> 
> What if Avocado introduces an option that would enable the use of
> Avocado's test dir when no test reference is given?  This could be
> enabled only once (but would be disabled by default):
> 
>   echo "[loader]" > ~/.config/avocado/avocado.conf
>   echo "use_test_dir_when_no_references_given = True" >> >
> ~/.config/avocado/avocado.conf
> 
>   echo "[datadir.paths]" >> ~/.config/avocado/avocado.conf
>   echo "test_dir = $source_path/tests/acceptance" >>
> ~/.config/avocado/avocado.conf
> 
> And after that, the following would run all "console" tests:
> 
>   avocado run -t console
> 
> How does this sound?

For my use cases this doesn't worry me, I'll let Eduardo/Fam opine about
use_test_dir_when_no_references_given.

> 
> - Cleber.
> 
>> but this doesn't work this way, I'd have to use
>>
>>   build_dir$ avocado --config .avocado.conf boot_linux_console.py -t console
>>
>> which isn't a win.
>> ---
>>  configure | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/configure b/configure
>> index a6a4616c3e..0763ebe5ba 100755
>> --- a/configure
>> +++ b/configure
>> @@ -7213,9 +7213,11 @@ for rom in seabios vgabios ; do
>>  done
>>  
>>  # set up tests data directory
>> -if [ ! -e tests/data ]; then
>> -    symlink "$source_path/tests/data" tests/data
>> -fi
>> +for tests_subdir in acceptance data; do
>> +    if [ ! -e tests/$tests_subdir ]; then
>> +        symlink "$source_path/tests/$tests_subdir" tests/$tests_subdir
>> +    fi
>> +done
>>  
>>  # set up qemu-iotests in this build directory
>>  iotests_common_env="tests/qemu-iotests/common.env"
>>

On Tue, Jun 05, 2018 at 11:45:03AM -0300, Philippe Mathieu-Daudé wrote:
[... something about config files ...]
> > And after that, the following would run all "console" tests:
> > 
> >   avocado run -t console
> > 
> > How does this sound?
> 
> For my use cases this doesn't worry me, I'll let Eduardo/Fam opine about
> use_test_dir_when_no_references_given.

Well, I can't give an opinion because I couldn't understand
what's the final goal here.

What exactly is missing in the current solution?  Why
"avocado run -t console ." wouldn't work?

-- 
Eduardo

On 06/06/2018 04:24 PM, Eduardo Habkost wrote:
> On Tue, Jun 05, 2018 at 11:45:03AM -0300, Philippe Mathieu-Daudé wrote:
> [... something about config files ...]
>>> And after that, the following would run all "console" tests:
>>>
>>>   avocado run -t console
>>>
>>> How does this sound?
>>
>> For my use cases this doesn't worry me, I'll let Eduardo/Fam opine about
>> use_test_dir_when_no_references_given.
> 
> Well, I can't give an opinion because I couldn't understand
> what's the final goal here.

You cut too much, the relevant part is:

  On 05/30/2018 10:06 PM, Cleber Rosa wrote:
  > But at the same, there are security implications: `list` won't
  > load/execute any test code (different from, say, standard Python
  > unittests), while "run" obviously will.  So "avocado run" may end up
  > running what users don't want if a malicious user controls
  > "$avocado_datadir_paths_test_dir".

> 
> What exactly is missing in the current solution?  Why
> "avocado run -t console ." wouldn't work?

It doesn't work in out-of-tree builds, I have to use the full path:

  >>   build_dir$ avocado run
/full/path/to/sources/qemu/tests/acceptance/boot_linux_console.py

Don't worry about this RFC patch, I don't have problem to use the full
path for now, I suppose the Avocado team will resolve this differently
via a configurable option.

Regards,

Phil.

On Wed, Jun 06, 2018 at 04:36:16PM -0300, Philippe Mathieu-Daudé wrote:
> On 06/06/2018 04:24 PM, Eduardo Habkost wrote:
> > On Tue, Jun 05, 2018 at 11:45:03AM -0300, Philippe Mathieu-Daudé wrote:
> > [... something about config files ...]
> >>> And after that, the following would run all "console" tests:
> >>>
> >>>   avocado run -t console
> >>>
> >>> How does this sound?
> >>
> >> For my use cases this doesn't worry me, I'll let Eduardo/Fam opine about
> >> use_test_dir_when_no_references_given.
> > 
> > Well, I can't give an opinion because I couldn't understand
> > what's the final goal here.
> 
> You cut too much, the relevant part is:
> 
>   On 05/30/2018 10:06 PM, Cleber Rosa wrote:
>   > But at the same, there are security implications: `list` won't
>   > load/execute any test code (different from, say, standard Python
>   > unittests), while "run" obviously will.  So "avocado run" may end up
>   > running what users don't want if a malicious user controls
>   > "$avocado_datadir_paths_test_dir".

The final goal still isn't clear to me.  Why would somebody want
to use $avocado_datadir_paths_test_dir instead of just specifying
"." in the command-line?


> 
> > 
> > What exactly is missing in the current solution?  Why
> > "avocado run -t console ." wouldn't work?
> 
> It doesn't work in out-of-tree builds, I have to use the full path:
> 
>   >>   build_dir$ avocado run
> /full/path/to/sources/qemu/tests/acceptance/boot_linux_console.py

Isn't the symlink you suggested a better solution than requiring
the user to edit a config file?

-- 
Eduardo

On 06/06/2018 04:11 PM, Eduardo Habkost wrote:
> On Wed, Jun 06, 2018 at 04:36:16PM -0300, Philippe Mathieu-Daudé wrote:
>> On 06/06/2018 04:24 PM, Eduardo Habkost wrote:
>>> On Tue, Jun 05, 2018 at 11:45:03AM -0300, Philippe Mathieu-Daudé wrote:
>>> [... something about config files ...]
>>>>> And after that, the following would run all "console" tests:
>>>>>
>>>>>   avocado run -t console
>>>>>
>>>>> How does this sound?
>>>>
>>>> For my use cases this doesn't worry me, I'll let Eduardo/Fam opine about
>>>> use_test_dir_when_no_references_given.
>>>
>>> Well, I can't give an opinion because I couldn't understand
>>> what's the final goal here.
>>
>> You cut too much, the relevant part is:
>>
>>   On 05/30/2018 10:06 PM, Cleber Rosa wrote:
>>   > But at the same, there are security implications: `list` won't
>>   > load/execute any test code (different from, say, standard Python
>>   > unittests), while "run" obviously will.  So "avocado run" may end up
>>   > running what users don't want if a malicious user controls
>>   > "$avocado_datadir_paths_test_dir".
> 
> The final goal still isn't clear to me.  Why would somebody want
> to use $avocado_datadir_paths_test_dir instead of just specifying
> "." in the command-line?
> 
> 
>>
>>>
>>> What exactly is missing in the current solution?  Why
>>> "avocado run -t console ." wouldn't work?
>>
>> It doesn't work in out-of-tree builds, I have to use the full path:
>>
>>   >>   build_dir$ avocado run
>> /full/path/to/sources/qemu/tests/acceptance/boot_linux_console.py
> 
> Isn't the symlink you suggested a better solution than requiring
> the user to edit a config file?
> 

It's definitely the simplest, and would avoid a questionable knob and
behavior in Avocado.

- Cleber.

On 06/06/2018 05:23 PM, Cleber Rosa wrote:
> On 06/06/2018 04:11 PM, Eduardo Habkost wrote:
>> On Wed, Jun 06, 2018 at 04:36:16PM -0300, Philippe Mathieu-Daudé wrote:
>>> On 06/06/2018 04:24 PM, Eduardo Habkost wrote:
>>>> On Tue, Jun 05, 2018 at 11:45:03AM -0300, Philippe Mathieu-Daudé wrote:
>>>> [... something about config files ...]
>>>>>> And after that, the following would run all "console" tests:
>>>>>>
>>>>>>   avocado run -t console
>>>>>>
>>>>>> How does this sound?
>>>>>
>>>>> For my use cases this doesn't worry me, I'll let Eduardo/Fam opine about
>>>>> use_test_dir_when_no_references_given.
>>>>
>>>> Well, I can't give an opinion because I couldn't understand
>>>> what's the final goal here.
>>>
>>> You cut too much, the relevant part is:
>>>
>>>   On 05/30/2018 10:06 PM, Cleber Rosa wrote:
>>>   > But at the same, there are security implications: `list` won't
>>>   > load/execute any test code (different from, say, standard Python
>>>   > unittests), while "run" obviously will.  So "avocado run" may end up
>>>   > running what users don't want if a malicious user controls
>>>   > "$avocado_datadir_paths_test_dir".
>>
>> The final goal still isn't clear to me.  Why would somebody want
>> to use $avocado_datadir_paths_test_dir instead of just specifying
>> "." in the command-line?

Oh I guess I misunderstood your first review, I'll just improve the
commit message and repost.

>>
>>
>>>
>>>>
>>>> What exactly is missing in the current solution?  Why
>>>> "avocado run -t console ." wouldn't work?
>>>
>>> It doesn't work in out-of-tree builds, I have to use the full path:
>>>
>>>   >>   build_dir$ avocado run
>>> /full/path/to/sources/qemu/tests/acceptance/boot_linux_console.py
>>
>> Isn't the symlink you suggested a better solution than requiring
>> the user to edit a config file?
>>
> 
> It's definitely the simplest, and would avoid a questionable knob and
> behavior in Avocado.

:)