Merge lp:~gocept/landscape-client/test-python3-ported into lp:~landscape/landscape-client/trunk

[Steffen Allner]

Run partial python3 tests (listed in file "py3_ready_tests") along with full python2 test run as part of "make check".

While we are porting more and more modules, we need a way to ensure, that already ported modules are not broken again. This MP introduces an additional trial run with using Python 3 in the normal check. The test which should be executed can be listed in a separate file.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Fail
Revno: 952
Branch: lp:~gocept/landscape-client/test-python3-ported
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3607/

review: Needs Fixing (test results)

[Steffen Allner (sallner)]

It seems, that the build system is not set up with the necessary packages to test under Python 3. It would be good to have the required packages installed, so that a failure of in already ported modules can be detected.

[Данило Шеган (danilo)]

The best way to ensure deps are available on all the builder slaves is to introduce a Makefile "depends" (make it .PHONY too) rule that simply does a "sudo apt install python3-twisted ...", and depend on that in the "check" rule.

[Данило Шеган (danilo)]

Great that you put this up: it's about time we start tracking python3 tests too!

Other than the above, a few nits on the makefile rules. However, I am also having trouble getting trial3 to work with -j4 on my system (which is what the CI builder will pass on): seems python-twisted package might be missing twisted.trial._dist module :/

For now, if CI fails with something like http://paste.ubuntu.com/24175376/, you can prepend TRIAL_ARGS= to the trial3 run.

review: Needs Fixing

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Fail
Revno: 953
Branch: lp:~gocept/landscape-client/test-python3-ported
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3613/

review: Needs Fixing (test results)

[Steffen Allner (sallner)]

For the distributed tests, it looks like the setup3.py of Twisted does not pack the trial._dist module. With version 16.x it seems Twisted has two different setup.py and the python3 one does not include it. In 17.x there is again only one, so you either patch that somehow or we live with the fact, that the Python 3 tests are not parallelised.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Fail
Revno: 954
Branch: lp:~gocept/landscape-client/test-python3-ported
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3614/

review: Needs Fixing (test results)

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 955
Branch: lp:~gocept/landscape-client/test-python3-ported
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3616/

review: Approve (test results)

[Данило Шеган (danilo)]

Right, let's ignore parallelisation of test runs in Python3 for now. We can work on that independently (probably just an omission in twisted).

As for the test, try by exporting an env var with UTF-8 locally before running a test:

  UNICODE_TEST=проба trial3 landscape/lib/tests/test_sysstats.py

I wonder if landscape-client will ever run in an environment containing non-ASCII strings, but I imagine it could. Note that this works in python2, so it's strictly an issue with Python 3 having os.environ as unicode strings instead of bytes (and it seems only when there is UTF-8 in the vars, otherwise they are bytes too).

Since the builder does not complain, we can get this landed, but it would be a tricky bug to chase down later if it happens to our users.

(Yeah, I am heavily using Serbian Cyrillic in all of our projects to stress test the Unicode support :-))

review: Approve

[Daniel Havlik (nilo)]

+1

review: Approve

[Steffen Allner (sallner)]

Actually I needed

  UNICODE_TEST=проба LC_ALL=C trial3 landscape/lib/tests/test_sysstats.py

to reproduce it. Looking at https://github.com/twisted/twisted/blob/49365f5c51d0e2dd6a808bfc7dee96946bba81a0/src/twisted/internet/base.py#L851 we really should enforce sending bytes in the environment.

One solution would be a try with sys.getdefaultencoding() as the twisted does it or with utf-8. If that fails an useful error message could be raised.

I would like to add this as a backlog item to the trello board of this project and resolve it in a separate branch. Is that okay for you? Then we can use the testing infrastructure already.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 956
Branch: lp:~gocept/landscape-client/test-python3-ported
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3619/

review: Approve (test results)

[Данило Шеган (danilo)]

Yeah, it's fine to do that as a separate task: please add a backlog item.

review: Approve

[🤖 Landscape Builder (landscape-builder)]

Voting does not meet specified criteria. Required: Approve >= 3, Disapprove == 0. Got: 2 Approve, 1 Pending.

=== modified file 'Makefile'
--- Makefile	2017-03-10 08:52:58 +0000
+++ Makefile	2017-03-14 11:03:31 +0000
@@ -4,6 +4,9 @@
 TRIAL_ARGS ?=
 TEST_COMMAND_PY2 = trial --unclean-warnings $(TRIAL_ARGS) landscape
 TEST_COMMAND_PY3 = trial3 --unclean-warnings $(TRIAL_ARGS) landscape
+READY_FILE := py3_ready_tests
+PY3_READY := `cat $(READY_FILE)`
+TEST_COMMAND_PY3_READY = TRIAL_ARGS= trial3 --unclean-warnings $(PY3_READY)
 UBUNTU_RELEASE := $(shell lsb_release -cs)
 # version in the code is authoritative
 # Use := here, not =, it's really important, otherwise UPSTREAM_VERSION
@@ -34,9 +37,17 @@
 check3: build3
 	LC_ALL=C $(TEST_COMMAND_PY3)
 
-check: build
+check: check2	check3-ready
+
+check2: build
 	LC_ALL=C $(TEST_COMMAND_PY2)
 
+check3-ready: depends3 build3
+	LC_ALL=C $(TEST_COMMAND_PY3_READY)
+
+depends3:
+	sudo apt -y install python3-twisted python3-distutils-extra python3-mock python3-configobj
+
 lint:
 	bzr ls-lint
 
@@ -129,4 +140,4 @@
 	cd sdist && md5sum landscape-client-$(TARBALL_VERSION).tar.gz > landscape-client-$(TARBALL_VERSION).tar.gz.md5
 	rm -rf sdist/landscape-client-$(TARBALL_VERSION)
 
-.PHONY: tags etags freshdata run freshrun package sourcepackage updateversion origtarball prepchangelog lint build check releasetarball
+.PHONY: tags etags freshdata run freshrun package sourcepackage updateversion origtarball prepchangelog lint build build3 check check2 check3 check3-ready check5 depends3 releasetarball

=== added file 'py3_ready_tests'
--- py3_ready_tests	1970-01-01 00:00:00 +0000
+++ py3_ready_tests	2017-03-14 11:03:31 +0000
@@ -0,0 +1,22 @@
+landscape/lib/tests/test_scriptcontent.py
+landscape/lib/tests/test_amp.py
+landscape/lib/tests/test_gpg.py
+landscape/lib/tests/test_sequenceranges.py
+landscape/lib/tests/test_bootstrap.py
+landscape/lib/tests/test_juju.py
+landscape/lib/tests/test_sysstats.py
+landscape/lib/tests/test_bpickle.py
+landscape/lib/tests/test_lock.py
+landscape/lib/tests/test_tag.py
+landscape/lib/tests/test_lsb_release.py
+landscape/lib/tests/test_timestamp.py
+landscape/lib/tests/test_disk.py
+landscape/lib/tests/test_monitor.py
+landscape/lib/tests/test_encoding.py
+landscape/lib/tests/test_versioning.py
+landscape/lib/tests/test_fd.py
+landscape/lib/tests/test_persist.py
+landscape/lib/tests/test_vm_info.py
+landscape/lib/tests/test_fetch.py
+landscape/lib/tests/test_process.py
+landscape/lib/tests/test_warning.py