Bug 1318765 - Review Request: openstack-sahara-tests - Sahara Scenario Test Framework
Summary: Review Request: openstack-sahara-tests - Sahara Scenario Test Framework
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: RDO
Classification: Community
Component: Package Review
Version: trunk
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
: trunk
Assignee: Alan Pevec
QA Contact: hguemar
URL:
Whiteboard:
Depends On:
Blocks: RDO-MITAKA, RDO-MITAKA-REVIEWS
TreeView+ depends on / blocked
 
Reported: 2016-03-17 18:16 UTC by Chandan Kumar
Modified: 2017-06-18 13:50 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-06-18 13:50:17 UTC
apevec: rdo-review+


Attachments (Terms of Use)

Comment 1 Luigi Toscano 2016-03-31 22:25:53 UTC
The only relevant error from rpmlint is non-conffile-in-etc, and this is come from the current upstream structure. This structure also has another drawback, as described below.

Regarding the functionality of the package, it is possible to execute the test runner (sahara-scenario binary) by explicitely passing the required templates as arguments. This  usually means the template for credentails, the one for jobs, and the plugin-specific one with cluster defintion.

But when you run the tests from the git repository, and the --plugin and --plugin_version parameters are used, the template files for credentials and jobs and plugin versions available in the source repository are automatically used without the user having to specify their paths.This is not possible right now in the package and probably not even by pip-installing the original package in a virtualenv, due to the way the discovery is done; see the value
TEST_TEMPLATE_DIR = 'etc/scenario/defaults/'
in /usr/lib/python2.7/site-packages/sahara_tests/scenario/runner.py
There is a WIP review upstream which addresses this: https://review.openstack.org/#/c/295110/.

I think that the package could go into DLRN in the meantime, holding back the promotion to stable Mitaka until this is fixed, but I'm not sure if this is possible according the review rules.

Comment 2 Luigi Toscano 2016-05-24 15:46:09 UTC
Any update on this? The newly released sahara-tests 0.2.0 should fix the issues outlined in the previous comment.

Comment 3 Alan Pevec 2016-05-25 13:38:29 UTC
Could we keep the name python-sahara-tests for the main package, obsoleting the python-sahara-tests subpackage of openstack-sahara?
Or are there still some tests left in the main repo?

Comment 4 Luigi Toscano 2016-05-25 14:23:03 UTC
I would keep the name. python-sahara-tests will always exist, with the unit tests for Sahara itself (and for now still the old tempest tests). This one contains scenario and tempest tests, but not all tests.

Comment 6 Alan Pevec 2016-06-01 17:09:59 UTC
I would make two subpackages out of sahara-tests, with empty main package, SRPM name would stay openstack-sahara-tests

* python-sahara-scenario from http://git.openstack.org/cgit/openstack/sahara-tests/tree/sahara_tests/scenario

* python-sahara-tempest from http://git.openstack.org/cgit/openstack/sahara-tests/tree/sahara_tempest_plugin

and existing -tests subpackages from in-project-tree, split them to:

* python-PROJECT-tests - with unit and functional tests
* python-PROJECT-tempest - for in-tree tempest plugin only

Bummer is that Sahara has _two_ tempest plugins :(
* sahara_clients_scenario_tests in-tree: https://github.com/openstack/sahara/blob/master/setup.cfg#L68
* sahara_tempest_tests in separate sahara-tests project: https://github.com/openstack/sahara-tests/blob/master/setup.cfg#L34 

Looks like sahara_clients_scenario_tests should actually be located in python-saharaclient project?

Comment 7 Luigi Toscano 2016-06-01 17:26:18 UTC
(In reply to Alan Pevec from comment #6)
> I would make two subpackages out of sahara-tests, with empty main package,
> SRPM name would stay openstack-sahara-tests
> 
> * python-sahara-scenario from
> http://git.openstack.org/cgit/openstack/sahara-tests/tree/sahara_tests/
> scenario
> 
> * python-sahara-tempest from
> http://git.openstack.org/cgit/openstack/sahara-tests/tree/
> sahara_tempest_plugin

So python-sahara-tests-tempest, for what it is below?
> 
> and existing -tests subpackages from in-project-tree, split them to:
> 
> * python-PROJECT-tests - with unit and functional tests
> * python-PROJECT-tempest - for in-tree tempest plugin only


> 
> Bummer is that Sahara has _two_ tempest plugins :(
> * sahara_clients_scenario_tests in-tree:
> https://github.com/openstack/sahara/blob/master/setup.cfg#L68
> * sahara_tempest_tests in separate sahara-tests project:
> https://github.com/openstack/sahara-tests/blob/master/setup.cfg#L34 
> 
> Looks like sahara_clients_scenario_tests should actually be located in
> python-saharaclient project?

No, they are meant to be branchless.

Sorry for that, but the alternative would have been two repositories, and that would become a bit overkill. The idea is to collect all higher level tests.

Comment 8 Alan Pevec 2016-06-01 23:36:20 UTC
> > Looks like sahara_clients_scenario_tests should actually be located in
> > python-saharaclient project?
> 
> No, they are meant to be branchless.

But then sahara_clients_scenario_tests should be moved out of openstack/sahara which is branch-full ?

> Sorry for that, but the alternative would have been two repositories, and
> that would become a bit overkill. The idea is to collect all higher level
> tests.

Mixing tempest plugin and scenarios in sahara-tests is fine, I'm complaining about two tempest plugins for sahara.
If those are meant to be branchless, why is there in-tree plugin, shouldn't that be moved to the branchless sahara-tests project?

Comment 9 Alan Pevec 2016-06-02 07:35:46 UTC
One more try at tests subpackages nomenclature:

* python-PROJECT-tests (as we have now) - includes all tests, and would be eventually virtual, requiring subpackages below for backward compatibility with the current state

* python-PROJECT-tests-unit - includes unit tests and their deps

* python-PROJECT-tests-tempest - Tempest plugin, requiring only deps for running Tempest test. Can be from subpackaged from in-tree plugin or top-level SRPM from a separate project.

Special cases for Sahara:
* python-sahara-tests-tempest-client - in-tree sahara_clients_scenario_tests Tempest plugin
* python-sahara-tests-tempest - sahara_tempest_tests Tempest plugin from sahara-tests project
* python-sahara-tests-scenario - sahara_tests/scenario

Comment 10 Luigi Toscano 2016-06-06 07:30:52 UTC
(In reply to Alan Pevec from comment #8)
> > > Looks like sahara_clients_scenario_tests should actually be located in
> > > python-saharaclient project?
> > 
> > No, they are meant to be branchless.
> 
> But then sahara_clients_scenario_tests should be moved out of
> openstack/sahara which is branch-full ?
Yes, but I'm waiting for some fixes on infra side. 

> 
> > Sorry for that, but the alternative would have been two repositories, and
> > that would become a bit overkill. The idea is to collect all higher level
> > tests.
> 
> Mixing tempest plugin and scenarios in sahara-tests is fine, I'm complaining
> about two tempest plugins for sahara.
> If those are meant to be branchless, why is there in-tree plugin, shouldn't
> that be moved to the branchless sahara-tests project?


Yes, they will, but not right now.

Comment 12 Luigi Toscano 2016-06-16 14:54:11 UTC
The generated openstack-sahara-tests contain most of the common code of scenario, which should go in fact to the -scenario package (including its tests and its dependencies). I understand that a common package is required to host the shared entry points, but them why not fold the releasenotes from -doc into openstack-sahara-tests and leave the latter almost empty (only releasenotes and entrypoints, all code either in -scenario or -tempest)?

Pending approval from packages, of course, not sure if it is according the packaging rules.

Comment 13 Alan Pevec 2016-07-05 08:37:40 UTC
ACK what tosky says, keep only egginfo and top-level site-packages/sahara_tests/ folder in the main package openstack-sahara-tests.
-doc is only required when there's large docs (few MBs) and there is actually NO content in https://github.com/openstack/sahara-tests/tree/master/doc/source/ - it's just generated sphinx boilerplate, so you could also just skip sphinx build too and drop -doc subpackage.

BTW releasenotes are not built, but that should be separate change across all openstack RPMs, TBD whether it's worth to subpackage or include into main.

One thing that fedora-review dug out: there's Java inside(TM) !
There are binary JARs under https://github.com/openstack/sahara-tests/tree/master/sahara_tests/scenario/defaults/edp-examples/ some have corresponding source, but looks like some are just copied from the
These could be treated as test fixtures, I'll add to RDO meeting packaging exception request, but strictly speaking everything should be buildable from source.

Comment 14 Luigi Toscano 2016-07-07 11:42:59 UTC
(In reply to Alan Pevec from comment #13)
> One thing that fedora-review dug out: there's Java inside(TM) !
> There are binary JARs under
> https://github.com/openstack/sahara-tests/tree/master/sahara_tests/scenario/
> defaults/edp-examples/ some have corresponding source, but looks like some
> are just copied from the
> These could be treated as test fixtures, I'll add to RDO meeting packaging
> exception request, but strictly speaking everything should be buildable from
> source.

This is an issue already reported by Debian packagers, but you can add other details of course: 
https://bugs.launchpad.net/sahara/+bug/1458853

Comment 17 Luigi Toscano 2016-07-18 17:14:34 UTC
The last(In reply to Chandan Kumar from comment #16)
> Here is the updated 
> SPEC FILE:
> https://raw.githubusercontent.com/chkumar246/openstack-sahara-tests/rpm-
> master/openstack-sahara-tests.spec
> 
> and SRPM
> :https://chandankumar.fedorapeople.org/sahara/openstack-sahara-tests-0.2.1-0.
> 20160718112130.914bf06.el7.centos.src.rpm

The issues highlighted in the previous comments has been addressed. The generated packages looks fine and working (scenario tests can be run, Tempest tests are picked up by the test runner). I think that it is good to go, in the worst case non-obvious issues, if found, can be fixed later.
I hit an error in the test phase while rebuilding the package, but it was on a "dirty" environment, not under mock, so it is probably not an issue and it will show up on koji.

Comment 19 Chandan Kumar 2016-09-12 15:05:32 UTC
Here is the rdoinfo review https://review.rdoproject.org/r/#/c/2200/

Comment 20 Alan Pevec 2016-09-13 09:57:18 UTC
Good for the initial import, nitpicks can be solved in the initial gerrit review:

* replace docs generation section with python build_sphinx
e.g. https://github.com/rdo-packages/nova-distgit/blob/8b2b91f1830f6d71c641077747c2cdbe74c70eac/openstack-nova.spec#L544
(this needs to be updated in https://github.com/openstack-packages/openstack-example-spec/blob/master/openstack-example.spec )

* shell scripts and other fixture should be moved out of pythonpath, but that should be taken upstream (there are similar issues in other tempest plugins, like writing in the pythonpath): %{python2_sitelib}/sahara_tests/scenario/defaults/edp-examples/edp-shell/shell-example.sh

*
# moving sahara-scenario and sahara_tempest_plugin
# to python-sahara-tempest and python-sahara-scenario package
^ subpackages are python-sahara-tests-tempest and python-sahara-tests-scenario

Comment 21 Alan Pevec 2016-09-13 10:17:03 UTC
> * shell scripts and other fixture should be moved out of pythonpath, but
> that should be taken upstream

> %{python2_sitelib}/sahara_tests/scenario/defaults/edp-examples/edp-shell/
> shell-example.sh

This was actually already discussed upstream, from tosky:
" they have been moved back there also to solve some detection issue
with them *outside* that location, the behavior as RPM and as pip package was different, and the code which find them failed in one of the two cases
the idea is that the sahara-scenario script should work out of the box
 https://review.openstack.org/#/c/305153/ followed by https://review.openstack.org/#/c/307702/
"

Comment 22 Chandan Kumar 2016-09-13 11:53:13 UTC
Initial Spec is imported in RDO : https://review.rdoproject.org/r/#/c/2225/

Comment 23 Alan Pevec 2016-09-14 13:01:04 UTC
Built in RDO Trunk (untested) https://trunk.rdoproject.org/centos7-master/consistent/

Comment 24 Christopher Brown 2017-06-18 13:50:17 UTC
This was landed.


Note You need to log in before you can comment on or make changes to this bug.