Re: [Qemu-devel] [PATCH 2/2] Acceptance tests: add make rule for running them

[Eduardo Habkost]

On Thu, Sep 20, 2018 at 04:00:27PM -0400, Cleber Rosa wrote:
> 
> 
> On 9/20/18 2:58 PM, Eduardo Habkost wrote:
> > On Thu, Sep 20, 2018 at 11:19:56AM -0400, Cleber Rosa wrote:
> >> The acceptance (aka functional, aka Avocado-based) tests are
> >> Python files located in "tests/acceptance" that need to be run
> >> with the Avocado libs and test runner.
> >>
> >> Let's provide a convenient way for QEMU developers to run them,
> >> by making use of the tests-venv with the required setup.
> >>
> >> Also, while the Avocado test runner will take care of creating a
> >> location to save test results to, it was understood that it's better
> >> if the results are kept within the build tree.
> >>
> >> Signed-off-by: Cleber Rosa <address@hidden>
> >> ---
> >>  docs/devel/testing.rst      | 28 +++++++++++++++++++++++-----
> >>  tests/Makefile.include      | 17 +++++++++++++++--
> >>  tests/venv-requirements.txt |  1 +
> >>  3 files changed, 39 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> >> index 727c4019b5..0fbf0d0aac 100644
> >> --- a/docs/devel/testing.rst
> >> +++ b/docs/devel/testing.rst
> >> @@ -545,10 +545,24 @@ Tests based on ``avocado_qemu.Test`` can easily:
> >>     - 
> >> http://avocado-framework.readthedocs.io/en/latest/api/test/avocado.html#avocado.Test
> >>     - 
> >> http://avocado-framework.readthedocs.io/en/latest/api/utils/avocado.utils.html
> >>  
> >> -Installation
> >> -------------
> >> +Running tests
> >> +-------------
> >>  
> >> -To install Avocado and its dependencies, run:
> >> +You can run the acceptance tests simply by executing:
> >> +
> >> +.. code::
> >> +
> >> +  make check-acceptance
> >> +
> >> +This involves the automatic creation of Python virtual environment
> >> +within the build tree (at ``tests/venv``) which will have all the
> >> +right dependencies, and will save tests results also within the
> >> +build tree (at ``tests/results``).
> >> +
> >> +Manual Installation
> >> +-------------------
> >> +
> >> +To manually install Avocado and its dependencies, run:
> >>  
> >>  .. code::
> >>  
> >> @@ -689,11 +703,15 @@ The exact QEMU binary to be used on QEMUMachine.
> >>  Uninstalling Avocado
> >>  --------------------
> >>  
> >> -If you've followed the installation instructions above, you can easily
> >> -uninstall Avocado.  Start by listing the packages you have installed::
> >> +If you've followed the manual installation instructions above, you can
> >> +easily uninstall Avocado.  Start by listing the packages you have
> >> +installed::
> >>  
> >>    pip list --user
> >>  
> >>  And remove any package you want with::
> >>  
> >>    pip uninstall <package_name>
> >> +
> >> +If you've used ``make check-acceptance``, the Python virtual environment 
> >> where
> >> +Avocado is installed will be cleaned up as part of ``make check-clean``.
> >> diff --git a/tests/Makefile.include b/tests/Makefile.include
> >> index 9bb90a83d4..8cef694954 100644
> >> --- a/tests/Makefile.include
> >> +++ b/tests/Makefile.include
> >> @@ -11,6 +11,7 @@ check-help:
> >>    @echo " $(MAKE) check-qapi-schema    Run QAPI schema tests"
> >>    @echo " $(MAKE) check-block          Run block tests"
> >>    @echo " $(MAKE) check-tcg            Run TCG tests"
> >> +  @echo " $(MAKE) check-acceptance     Run all acceptance (functional) 
> >> tests"
> >>    @echo " $(MAKE) check-report.html    Generates an HTML test report"
> >>    @echo " $(MAKE) check-venv           Creates a Python venv for tests"
> >>    @echo " $(MAKE) check-clean          Clean the tests"
> >> @@ -1002,10 +1003,11 @@ check-decodetree:
> >>  
> >>  # Python venv for running tests
> >>  
> >> -.PHONY: check-venv
> >> +.PHONY: check-venv check-acceptance
> >>  
> >>  TESTS_VENV_DIR=$(BUILD_DIR)/tests/venv
> >>  TESTS_VENV_REQ=$(BUILD_DIR)/tests/venv-requirements.txt
> >> +TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results
> >>  
> >>  $(TESTS_VENV_DIR):
> >>    $(call quiet-command, \
> >> @@ -1015,8 +1017,19 @@ $(TESTS_VENV_DIR):
> >>              $(TESTS_VENV_DIR)/bin/pip -q install -r $(TESTS_VENV_REQ), \
> >>              PIP, $(TESTS_VENV_REQ))
> >>  
> >> +$(TESTS_RESULTS_DIR):
> >> +  $(call quiet-command, mkdir -p $@, \
> >> +            MKDIR, $@)
> >> +
> >>  check-venv: $(TESTS_VENV_DIR)
> >>  
> >> +check-acceptance: check-venv $(TESTS_RESULTS_DIR)
> >> +  $(call quiet-command, \
> >> +            $(TESTS_VENV_DIR)/bin/avocado \
> >> +            --show=none run --job-results-dir=$(TESTS_RESULTS_DIR) 
> >> --failfast=on \
> >> +            $(SRC_PATH)/tests/acceptance, \
> >> +            "AVOCADO", "tests/acceptance")
> > 
> > I think we should provide something easy to use for people who
> > already have the right Avocado version installed in their system
> > and want to avoid re-downloading Avocado every time.
> > 
> 
> Right now, people using their own Avocado installation is actually the
> documented way.  The difference from the currently documented way is
> that instead of doing `make check-acceptance`, people will run:
> 
>  $ avocado run tests/acceptance
> 
> IMO, for these users, a `alias check-acceptance='avocado run
> tests/acceptance'` brings almost the same value.
> 
> About re-downloading: pip caches files by default, so while Avocado will
> be installed every time a new venv is created, it should be downloaded
> only once.  And I should mention that, given the fact that one of the
> packaged formats of Avocado is a "Python wheel", the installation is
> basically a "tar xf" of sorts.

Fair enough.  Note that I'm just guessing what other developers
would expect here.  Maybe most people won't mind having "pip
install" running implicitly when they run acceptance tests and
this is a non-issue.

I'm hoping we can get the attention of more people on this thread
so we can get feedback from actual users.  If we don't get any
feedback about this, I won't mind if we include only the rule you
suggested, and improve the system later.


> 
> > We already have plans to do this automatically/transparently in
> > the future, but maybe while we don't have something automatic we
> > could have two separate rules.  e.g.:
> > 
> >   AVOCADO = avocado
> > 
> >   check-acceptance: $(TESTS_RESULTS_DIR)
> >     $(call quiet-command, \
> >               $(AVOCADO) \
> >               --show=none run --job-results-dir=$(TESTS_RESULTS_DIR) 
> > --failfast=on \
> >               $(SRC_PATH)/tests/acceptance, \
> >               "AVOCADO", "tests/acceptance")
> > 
> >   check-acceptance-venv: check-venv
> >     $(MAKE) check-acceptance AVOCADO=$(TESTS_VENV_DIR)/bin/avocado
> > 
> > 
> 
> Yep, this could easily be done.  But, to me, the beauty of `make
> check-acceptance` not using a configurable venv, is that we can more
> easily pin down the origin of failures.  Notice how I went with
> "avocado-framework==64.0", and how in another patch (the boot_linux
> test), I mentioned adding "pycdlib==1.6.0".
> 
> If `make check-acceptance` is not configurable, we have a lot of
> questions that we don't have to ask with regards to the environment used
> and the possible causes of failures.
> 
> What do you think?

Very good point.  Just blindly trying to use the system-provided
packages is likely to be a problem, so ignore my suggestion by
now.  If we want to use system-provided modules, we must validate
them somehow before trying to run the tests.

Maybe all of this will work out of the box if we just use
system_site_packages=True?  What would happen if
system_site_packages=True and the system has a too old version of
Avocado installed?  How would 'pip' behave if the system already
has all the right dependencies installed?



> 
> - Cleber.
> 
> >> +
> >>  # Consolidated targets
> >>  
> >>  .PHONY: check-qapi-schema check-qtest check-unit check check-clean
> >> @@ -1030,7 +1043,7 @@ check-clean:
> >>    rm -rf $(check-unit-y) tests/*.o $(QEMU_IOTESTS_HELPERS-y)
> >>    rm -rf $(sort $(foreach target,$(SYSEMU_TARGET_LIST), 
> >> $(check-qtest-$(target)-y)) $(check-qtest-generic-y))
> >>    rm -f tests/test-qapi-gen-timestamp
> >> -  rm -rf $(TESTS_VENV_DIR)
> >> +  rm -rf $(TESTS_VENV_DIR) $(TESTS_RESULTS_DIR)
> >>  
> >>  clean: check-clean
> >>  
> >> diff --git a/tests/venv-requirements.txt b/tests/venv-requirements.txt
> >> index d39f9d1576..1734d0ce27 100644
> >> --- a/tests/venv-requirements.txt
> >> +++ b/tests/venv-requirements.txt
> >> @@ -1,3 +1,4 @@
> >>  # Add Python module requirements, one per line, to be installed
> >>  # in the tests/venv Python virtual environment. For more info,
> >>  # refer to: https://pip.pypa.io/en/stable/user_guide/#id1
> >> +avocado-framework==64.0
> >> -- 
> >> 2.17.1
> >>
> > 
> 
> -- 
> Cleber Rosa
> [ Sr Software Engineer - Virtualization Team - Red Hat ]
> [ Avocado Test Framework - avocado-framework.github.io ]
> [  7ABB 96EB 8B46 B94D 5E0F  E9BB 657E 8D33 A5F2 09F3  ]

-- 
Eduardo