test: only test enabled python bindings #21293

tchaikov · 2018-04-09T05:01:01Z

we should test the python version of MGR_PYTHON_VERSION_MAJOR, if we are
testing both py2.7 and py3, we are preparing for distributing dashboard
as a separated package independent of ceph-mgr.

rjfd · 2018-04-09T06:34:23Z

@tchaikov make check failed but it doesn't look like it's because of the changes in this PR.

tchaikov · 2018-04-09T07:20:54Z

retest this please.

smithfarm · 2018-04-09T12:52:25Z

src/pybind/mgr/dashboard/run-tox.sh

 fi
+ENV_LIST+="cov-report,lint"


So, the change here is that when WITH_PYTHON2=ON and WITH_PYTHON3=OFF, py3 is not included in ENV_LIST

That seems reasonable to me

actually, the change is to get the test pass, even if WITH_PYTHON2=OFF and WITH_PYTHON3=ON.

sebastian-philipp

Looks sane to me. But haven't tested the code.

rjfd · 2018-04-09T13:07:55Z

src/pybind/mgr/dashboard/run-tox.sh

@@ -17,11 +17,14 @@ fi

 source ${MGR_DASHBOARD_VIRTUALENV}/bin/activate

+ENV_LIST="cov-init,"
+if [ "$WITH_PYTHON2" = "ON" ]; then


Shouldn't we declared a default value for WITH_PYTHON2 in the beginning of the script (as we do with WITH_PYTHON3) so that the script runs well outside ctest.

@rjfd fixed and repushed.

rjfd · 2018-04-11T06:44:50Z

@tchaikov I tested this PR in my environment where I have WITH_PYTHON2=OFF and there is still one small thing we need to fix. The "lint" env is running always in python 2 and fails because I don't have the rbd and rados python bindings built for python 2. I created this patch that solves this issue:

diff --git a/src/pybind/mgr/dashboard/run-tox.sh b/src/pybind/mgr/dashboard/run-tox.sh
index 564c75b308..fe11683245 100755
--- a/src/pybind/mgr/dashboard/run-tox.sh
+++ b/src/pybind/mgr/dashboard/run-tox.sh
@@ -25,7 +25,13 @@ fi
 if [ "$WITH_PYTHON3" = "ON" ]; then
   ENV_LIST+="py3,"
 fi
-ENV_LIST+="cov-report,lint"
+ENV_LIST+="cov-report,"
+if [ "$WITH_PYTHON2" = "ON" ]; then
+  ENV_LIST+="lint2"
+fi
+if [ "$WITH_PYTHON3" = "ON" ]; then
+  ENV_LIST+="lint3"
+fi
 
 tox -c ${TOX_PATH} -e $ENV_LIST
 
diff --git a/src/pybind/mgr/dashboard/tox.ini b/src/pybind/mgr/dashboard/tox.ini
index 743a8a6929..9b07e15e8e 100644
--- a/src/pybind/mgr/dashboard/tox.ini
+++ b/src/pybind/mgr/dashboard/tox.ini
@@ -1,5 +1,5 @@
 [tox]
-envlist = cov-init,py27,py3,cov-report,lint
+envlist = cov-init,py27,py3,cov-report,lint2,lint3
 skipsdist = true
 
 [testenv]
@@ -30,9 +30,20 @@ commands =
     coverage report
     coverage xml
 
-[testenv:lint]
+[testenv:lint2]
+basepython=python2
 setenv =
-    PYTHONPATH = {toxinidir}/../../../../build/lib/cython_modules/lib.3:{toxinidir}/../../../../build/lib/cython_modules/lib.2
+    PYTHONPATH = {toxinidir}/../../../../build/lib/cython_modules/lib.2
+    LD_LIBRARY_PATH = {toxinidir}/../../../../build/lib
+deps=-r{toxinidir}/requirements.txt
+commands=
+    pylint --rcfile=.pylintrc --jobs=5 . module.py tools.py controllers tests services
+    pycodestyle --max-line-length=100 --exclude=python2.7,.tox,venv,frontend --ignore=E402,E121,E123,E126,E226,E24,E704,W503 .
+
+[testenv:lint3]
+basepython=python3
+setenv =
+    PYTHONPATH = {toxinidir}/../../../../build/lib/cython_modules/lib.3
     LD_LIBRARY_PATH = {toxinidir}/../../../../build/lib
 deps=-r{toxinidir}/requirements.txt
 commands=

Can you apply it to this PR?

tchaikov · 2018-04-11T11:14:04Z

@rjfd i restructured the tox.ini to dedup the settings. could you take another look? thanks!

rjfd · 2018-04-11T11:35:50Z

src/pybind/mgr/dashboard/tox.ini

-deps=-r{toxinidir}/requirements.txt
+basepython =
+    py27: python2.7
+    py3: python3


For some unknown reason, the virtual environments are being created outside of .tox directory, please add this additional line to fix it:
envdir = {toxinidir}/.tox/{envname}

@rjfd it's on purpose. i wanted to avoid creating the venvs in the source directory. see https://github.com/ceph/ceph/pull/21293/files#diff-cc2ee9d8e56f3a2cd98b8148935d3829R28. why do you think we should put them into .tox?

I see. I usually run run-tox.sh manually in the source directory and I was getting unstaged changes in my repo because of those directories. I was suggesting to move them into .tox directory because .tox is in the .gitignore.

Instead of creating them inside .tox, we can add the virtualenv directories to the .gitignore?

@rjfd hmm, how about this:

add a default value for CEPH_BUILD_DIR in run-tox.sh to point it to $PWD/.tox

?

anyway, IMHO, ctest is a more handy way to run tests.

rjfd · 2018-04-11T11:36:31Z

src/pybind/mgr/dashboard/tox.ini

+    cov: coverage report
+    cov: coverage xml
+    lint: pylint --rcfile=.pylintrc --jobs=5 . module.py tools.py controllers tests services
+    lint: pycodestyle --max-line-length=100 --exclude=python2.7,.tox,venv,frontend --ignore=E402,E121,E123,E126,E226,E24,E704,W503 .


I think you can remove the python2.7 from the exclude filter.

tchaikov · 2018-04-11T12:17:14Z

changelog

dropped basepython setting from testenv. because tox set it automatically.
dropped python2.7 from --exclude

@rjfd fixed and repushed.

we should test the python version of MGR_PYTHON_VERSION_MAJOR, if we are testing both py2.7 and py3, we are preparing for distributing dashboard as a separated package independent of ceph-mgr. restructure the dashboard tests as 2 matrices, so we have 2*2 tests for coverage and lint respectively. Signed-off-by: Kefu Chai <kchai@redhat.com>

unlike ceph-disk and ceph-detect-init, dashboard is not a standalone python application, it is a python application hosted by ceph-mgr, so no need to create a venv in which it is deployed. the python env created by tox would suffice. Signed-off-by: Kefu Chai <kchai@redhat.com>

tchaikov · 2018-04-11T14:31:34Z

changelog

add default value of CEPH_BUILD_DIR
add the commit to drop MGR_DASHBOARD_VIRTUALENV for testing dashboard

rjfd

LGTM

this silences following errors: 9: ./awsauth.py:42:1: E722 do not use bare except' 9: ./awsauth.py:116:13: E722 do not use bare except' Signed-off-by: Kefu Chai <kchai@redhat.com>

tchaikov · 2018-04-13T09:44:15Z

http://pulpito.ceph.com/kchai-2018-04-12_15:56:07-rados-wip-kefu-testing-2018-04-12-2211-distro-basic-mira/

failed tests are

tracked by http://tracker.ceph.com/issues/23699
caused by include/types: format decimal numbers with decimal factor #19117, mon,mgr: improve 'mgr module disable' cmd #21188
fixed by qa/suites/rados: enlarge osd-max-scrubs #21406

votdev · 2018-04-13T12:37:28Z

This PR causes a make check problem, see http://tracker.ceph.com/issues/23709.

tchaikov · 2018-04-13T13:09:48Z

#21416 should address this problem.

tchaikov added cleanup build/ops labels Apr 9, 2018

tchaikov requested a review from rjfd April 9, 2018 05:01

tchaikov force-pushed the wip-no-venv-for-dashboard branch 4 times, most recently from f8a337d to f28a096 Compare April 9, 2018 10:14

tchaikov requested review from smithfarm and removed request for rjfd April 9, 2018 11:58

tchaikov changed the title ~~cmake: no need to setup venv for dashboard~~ test: only test enabled python bindings Apr 9, 2018

tchaikov force-pushed the wip-no-venv-for-dashboard branch from f28a096 to 705980f Compare April 9, 2018 11:59

tchaikov added tests and removed build/ops labels Apr 9, 2018

smithfarm requested review from rjfd, sebastian-philipp and tspmelo April 9, 2018 12:49

smithfarm reviewed Apr 9, 2018

View reviewed changes

sebastian-philipp approved these changes Apr 9, 2018

View reviewed changes

rjfd reviewed Apr 9, 2018

View reviewed changes

tchaikov force-pushed the wip-no-venv-for-dashboard branch from 705980f to 0d1ad13 Compare April 10, 2018 11:19

tchaikov force-pushed the wip-no-venv-for-dashboard branch 2 times, most recently from 9635f30 to 473eacb Compare April 11, 2018 11:13

rjfd suggested changes Apr 11, 2018

View reviewed changes

tchaikov force-pushed the wip-no-venv-for-dashboard branch from 473eacb to adbab1b Compare April 11, 2018 12:16

tchaikov force-pushed the wip-no-venv-for-dashboard branch from adbab1b to 213652a Compare April 11, 2018 14:25

rjfd approved these changes Apr 11, 2018

View reviewed changes

pybind/mgr/dashboard: silence lint warnings

c5a004d

this silences following errors: 9: ./awsauth.py:42:1: E722 do not use bare except' 9: ./awsauth.py:116:13: E722 do not use bare except' Signed-off-by: Kefu Chai <kchai@redhat.com>

tchaikov added needs-qa wip-kefu-testing labels Apr 11, 2018

tchaikov merged commit d02710d into ceph:master Apr 13, 2018

tchaikov deleted the wip-no-venv-for-dashboard branch April 13, 2018 09:45

LenzGr added the dashboard label Apr 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: only test enabled python bindings #21293

test: only test enabled python bindings #21293

tchaikov commented Apr 9, 2018 •

edited

rjfd commented Apr 9, 2018

tchaikov commented Apr 9, 2018

smithfarm Apr 9, 2018

tchaikov Apr 9, 2018 •

edited

sebastian-philipp left a comment

rjfd Apr 9, 2018

tchaikov Apr 10, 2018

rjfd commented Apr 11, 2018

tchaikov commented Apr 11, 2018

rjfd Apr 11, 2018

tchaikov Apr 11, 2018

rjfd Apr 11, 2018

tchaikov Apr 11, 2018

rjfd Apr 11, 2018

tchaikov commented Apr 11, 2018 •

edited

tchaikov commented Apr 11, 2018

rjfd left a comment

tchaikov commented Apr 13, 2018 •

edited

votdev commented Apr 13, 2018

tchaikov commented Apr 13, 2018

		fi
		ENV_LIST+="cov-report,lint"

test: only test enabled python bindings #21293

test: only test enabled python bindings #21293

Conversation

tchaikov commented Apr 9, 2018 • edited

rjfd commented Apr 9, 2018

tchaikov commented Apr 9, 2018

Choose a reason for hiding this comment

tchaikov Apr 9, 2018 • edited

Choose a reason for hiding this comment

sebastian-philipp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjfd commented Apr 11, 2018

tchaikov commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Apr 11, 2018 • edited

tchaikov commented Apr 11, 2018

rjfd left a comment

Choose a reason for hiding this comment

tchaikov commented Apr 13, 2018 • edited

votdev commented Apr 13, 2018

tchaikov commented Apr 13, 2018

tchaikov commented Apr 9, 2018 •

edited

tchaikov Apr 9, 2018 •

edited

tchaikov commented Apr 11, 2018 •

edited

tchaikov commented Apr 13, 2018 •

edited