Test Reorganization

[holmanb]

Proposed Commit Message

Reorganize unit test locations under tests/unittests

This attempts to standardize unit test file location under test/unittests/
such that any source file located at cloudinit/path/to/file.py may have a
corresponding unit test file at test/unittests/path/to/test_file.py.

Noteworthy Comments:
====================
Four different duplicate test files existed:
test_{gpg,util,cc_mounts,cc_resolv_conf}.py
Each of these duplicate file pairs has been merged together. This is a
break in git history for these files.

The test suite appears to have a dependency on test order. Changing test
order causes some tests to fail. This should be rectified, but for now some
tests have been modified in tests/unittests/config/test_set_passwords.py.

A helper class name starts with "Test" which causes pytest to try executing it
as a test case, which then throws warnings "due to Class having __init__()".
Silence by changing the name of the class.

# helpers.py is imported in many test files, import paths change accordingly
cloudinit/tests/helpers.py -> tests/unittests/helpers.py

# Move directories:
cloudinit/distros/tests -> tests/unittests/distros
cloudinit/cmd/devel/tests -> tests/unittests/cmd/devel
cloudinit/cmd/tests -> tests/unittests/cmd/
cloudinit/sources/helpers/tests -> tests/unittests/sources/helpers
cloudinit/sources/tests -> tests/unittests/sources
cloudinit/net/tests -> tests/unittests/net
cloudinit/config/tests -> tests/unittests/config
cloudinit/analyze/tests/ -> tests/unittests/analyze/

# Standardize tests already in tests/unittests/
test_datasource -> sources
test_distros -> distros
test_vmware -> sources/vmware
test_handler -> config        # this contains cloudconfig module tests
test_runs -> runs               

More notes/context

• Some changes were repetitive; a fair amount of sed/awk/find -exec/etc was used to generate this PR
• The same number of tests pass before and after this change.

Checklist:

• My code follows the process laid out in the documentation
• I have updated or added any unit tests accordingly
• I have updated or added any documentation accordingly

[TheRealFalcon]

Thanks for this! This has been bothering me since forever 🙂

Given how large util.py is (which is its own problem), I think I'm ok with one giant test_util.py file. Ideally they'd both be smaller, but arbitrarily splitting the tests seems like it'd just create more confusion with no real benefit other than to avoid large files.

Standardize names and locations of tests already in tests/unittests/
test_datasource -> sources
test_distros -> distros
test_vmware -> sources/vmware
test_handler -> config

Big +1

test_runs -> runs # I don't understand fully what these tests are testing

Yeah, I don't think they're named very well. They're basically higher level tests of cloudinit/stages.py . Almost like a super mocked integration test. I think runs is fine.

Can we use MockDistro instead of DistroTester? DistroTester makes me think it would do something to test distros, but it acts as a standin distro for tests that require a distro being passed but don't actually do anything to test the distro.

+1 to everything else you've changed/proposed.

Let's try to get this merged ASAP so you can avoid rebase hell.

[holmanb]

Given how large util.py is (which is its own problem), I think I'm ok with one giant test_util.py file. Ideally they'd both be smaller, but arbitrarily splitting the tests seems like it'd just create more confusion with no real benefit other than to avoid large files.

Ack

Can we use MockDistro instead of DistroTester? DistroTester makes me think it would do something to test distros, but it acts as a standin distro for tests that require a distro being passed but don't actually do anything to test the distro.

Of course, thanks!

+1 to everything else you've changed/proposed.

Let's try to get this merged ASAP so you can avoid rebase hell.

Agreed.

Thanks for the review @TheRealFalcon!

[holmanb]

@TheRealFalcon In addition to your requested changes, I pushed a change that standardizes the config module testfile names. While doing that I found a couple more duplicate test files and merged them together.

@blackboxsw Do you want to take a look at this?

[TheRealFalcon]

LGTM!

[blackboxsw]

I agree with your consolidation here under the premise that more frequent than not folks are trying to find code and seek out call sites in functional logic. We, upstream, never got around to pushing all tests to a module-relative ./tests subdirectory fully before the motivation on my side was not strong enough to make that move permanent.

The original concern we were trying to address was what you have remedied here with your directory renames:

tests/unittests subdirectory names didn't represent the same directory structure we see in the code itself. So, folks were confused in which test module to add unit tests.

One we we could have solved this was to mandate module-relative tests, but properly restructuring the directories and test names under tests/unittests is probably more valuable to reduce complexity in development, testing and packaging as well.

Some things I think we can add to this PR:

1. git rm tests/unittests/test_handler tests/unittests/test_datasource/ tests/unittests/test_vmware tests/unittests/test_filters tests/unittests/test_distros tests/unittests/test_runs
2. Update doc/rtd/topics/testing.rst to redact anything talking about module-local tests subdirectory and point people to tests/unittests

[TheRealFalcon]

1. git rm tests/unittests/test_handler tests/unittests/test_datasource/ tests/unittests/test_vmware tests/unittests/test_filters tests/unittests/test_distros tests/unittests/test_runs

Git doesn't track directories, so I don't think there's anything else to do here. On my machine, I don't see those directories left. Do you?

[blackboxsw]

1. tests/unittests/test_handler tests/unittests/test_datasource/ tests/unittests/test_vmware tests/unittests/test_filters tests/unittests/test_distros tests/unittests/test_runs

Oopsie, I had stale pycache files from an unrelated checkout which left my empty subdirectories intact.
The clean pull doesn't represent the empty dirs you are right.

A couple of additional comments I think now that all tests out of cloudinit module subdirs:

1. We can drop cloudinit paths from pytest-related args in tox.ini since we should have no unit tests there anymore
2. setup.py no longer needs to exclude tests from every subdirectory during packaging so our excludes can be dropped

Also if we are touching tox.ini we could drop the old Xenial support directives.

diff --git a/setup.py b/setup.py
index 58fddf0f..1ee9bf0d 100755
--- a/setup.py
+++ b/setup.py
@@ -291,7 +291,7 @@ setuptools.setup(
     author='Scott Moser',
     author_email='scott.moser@canonical.com',
     url='http://launchpad.net/cloud-init/',
-    packages=setuptools.find_packages(exclude=['tests.*', '*.tests', 'tests']),
+    packages=setuptools.find_packages(]),
     scripts=['tools/cloud-init-per'],
     license='Dual-licensed under GPLv3 or Apache 2.0',
     data_files=data_files,
diff --git a/tox.ini b/tox.ini
index 874d3f20..3337763d 100644
--- a/tox.ini
+++ b/tox.ini
@@ -3,7 +3,7 @@ envlist = py3, xenial-dev, flake8, pylint
 recreate = True
 
 [testenv]
-commands = {envpython} -m pytest {posargs:tests/unittests cloudinit}
+commands = {envpython} -m pytest {posargs:tests/unittests}
 setenv =
     LC_ALL = en_US.utf-8
 passenv=
@@ -37,7 +37,7 @@ deps =
 commands = {envpython} -m pytest \
             --durations 10 \
             {posargs:--cov=cloudinit --cov-branch \
-            tests/unittests cloudinit}
+            tests/unittests}
 
 [testenv:py27]
 basepython = python2.7
@@ -162,9 +162,8 @@ setenv =
     PYTEST_ADDOPTS="-m not adhoc"
 
 [pytest]
-# TODO: s/--strict/--strict-markers/ once xenial support is dropped
-testpaths = cloudinit tests/unittests
-addopts = --strict
+testpaths = tests/unittests
+addopts = --strict-markers
 log_format = %(asctime)s %(levelname)-9s %(name)s:%(filename)s:%(lineno)d %(message)s
 log_date_format = %Y-%m-%d %H:%M:%S
 markers =

[TheRealFalcon]

IIRC we left the Xenial references in tox because it was the easiest way to test 3.5 support. Let's leave that reference until we officially drop 3.5, but +1 to all of Chad's other comments.

[holmanb]

2. setup.py no longer needs to exclude tests from every subdirectory during packaging so our excludes can be dropped

diff --git a/setup.py b/setup.py
index 58fddf0f..1ee9bf0d 100755
--- a/setup.py
+++ b/setup.py
@@ -291,7 +291,7 @@ setuptools.setup(
     author='Scott Moser',
     author_email='scott.moser@canonical.com',
     url='http://launchpad.net/cloud-init/',
-    packages=setuptools.find_packages(exclude=['tests.*', '*.tests', 'tests']),
+    packages=setuptools.find_packages(]),
     scripts=['tools/cloud-init-per'],
     license='Dual-licensed under GPLv3 or Apache 2.0',
     data_files=data_files,

We still need at least tests.* and test in the exclude list. This function is actually returning a list of packages, not a list of files. I agree we can remove '*.tests' from the list, since there shouldn't be any .tests packages in the source tree to filter out.

Per the setuptools website :

(find_packages) takes a source directory and two lists of package name patterns to exclude and include, and then return a list of str representing the packages it could find

also compare without an exclude list:

(Pdb) setuptools.find_packages()
['cloudinit', 'tests', 'cloudinit.filters', 'cloudinit.reporting', 'cloudinit.analyze', 'cloudinit.handlers', 'cloudinit.net', 'cloudinit.mergers', 'cloudinit.config', 'cloudinit.distros', 'cloudinit.cmd', 'cloudinit.sources', 'cloudinit.distros.parsers', 'cloudinit.cmd.devel', 'cloudinit.sources.helpers', 'cloudinit.sources.helpers.vmware', 'cloudinit.sources.helpers.vmware.imc', 'tests.integration_tests', 'tests.unittests', 'tests.unittests.filters', 'tests.unittests.net', 'tests.unittests.config', 'tests.unittests.distros', 'tests.unittests.cmd', 'tests.unittests.runs', 'tests.unittests.sources', 'tests.unittests.cmd.devel', 'tests.unittests.sources.vmware']

versus with tests.* and tests:

(Pdb) setuptools.find_packages(exclude=['tests.*', 'tests'])
['cloudinit', 'cloudinit.filters', 'cloudinit.reporting', 'cloudinit.analyze', 'cloudinit.handlers', 'cloudinit.net', 'cloudinit.mergers', 'cloudinit.config', 'cloudinit.distros', 'cloudinit.cmd', 'cloudinit.sources', 'cloudinit.distros.parsers', 'cloudinit.cmd.devel', 'cloudinit.sources.helpers', 'cloudinit.sources.helpers.vmware', 'cloudinit.sources.helpers.vmware.imc']

The first contains various tests.unittests.* and tests.integration_tests.* packages that we don't want.

[TheRealFalcon]

+1 from me. I think we just need a final approval from @blackboxsw

[blackboxsw]

Thank you for the cleanup and correcting my assumptions on setup.py. Validated the final deb created continues to avoid packaging tests dirs and test runner continues to hit all tests appropriately.

LGTM!

[holmanb]

@blackboxsw @TheRealFalcon - Big thanks to you both for the reviews and support on this!