cloudinit: move to pytest for running tests

[OddBloke]

As the nose docs[0] themselves note, it has been in maintenance mode for the past several years. pytest is an actively developed, featureful and popular alternative that the nose docs themselves recommend. See [1] for more details about the thinking here.

(This PR also removes stale tox definitions, instead of modifying them.)

[0] https://nose.readthedocs.io/en/latest/
[1] https://lists.launchpad.net/cloud-init/msg00245.html

[smoser]

Is there a reason you pushed this branch to canonical/cloud-init rather than oddbloke's fork?
I'd suggest that best practice would be to not have push access to the repo, or at very least for your development system to use the read-only for the upstream remote.

[smoser]

It feels like you need some changes to packages/pkg-deps.json which might fall out if you try to use tools/run-container.

[smoser]

I did a quick test, and 'python -m pytest' works here.
That would mean we should be able to use a consistent 'commands' line of:

{envpython} -m pytest ....

[smoser]

Can you change this to --cov=cloudinit ? It makes it obvious that cloud-init is a value to the --cov key, rather than some other thing.

[OddBloke]

On Fri, Feb 14, 2020 at 08:45:07AM -0800, Scott Moser wrote: Is there a reason you pushed this branch to canonical/cloud-init rather than oddbloke's fork?

It meant that Travis CI would run against it without me having to open up a PR (which I didn't want to do until this was at least close to ready for review).

I'd suggest that best practice would be to not have push access to the repo, or at very least for your development system to use the read-only for the upstream remote.

We have branch protection[0] enabled on master, so GitHub will not allow pushes to that at all, we can only land commits there via pull request. If you'd prefer for us to be more restrictive, can you start a discussion on the cloud-init list? (Note that we _do_ use write access to the upstream repo to push exactly the commit that was uploaded to packaging branches.) [0] https://help.github.com/en/github/administering-a-repository/configuring-protected-branches

[OddBloke]

On Fri, Feb 14, 2020 at 09:04:39AM -0800, Scott Moser wrote: It feels like you need some changes to packages/pkg-deps.json which might fall out if you try to use tools/run-container.

I can't work out an invocation that will actually find an image: ``` $ ./tools/run-container -u centos/7 Creating ci-51e4c01a Error: Failed to fetch https://us.images.linuxcontainers.org:8443/1.0/images/centos/7: 404 Not Found Failed to start container 'ci-51e4c01a' from 'images:centos/7' ret=1 Failed to start container for images:centos/7 ret=1 ``` I've tried various different invocations for CentOS and Ubuntu, and they all failed. So I'm not able to test this change locally.

> @@ -65,7 +64,7 @@ commands = [testenv:xenial] commands = python ./tools/pipremove jsonschema - python -m nose {posargs:tests/unittests cloudinit} + py.test {posargs:tests/unittests cloudinit} I did a quick test, and 'python -m pytest' works here. That would mean we should be able to use a consistent 'commands' line of: {envpython} -m pytest ....

Good point, changed everywhere.

> -r{toxinidir}/test-requirements.txt -commands = {envpython} -m nose --with-timer --timer-top-n 10 \ - {posargs:--with-coverage --cover-erase --cover-branches \ - --cover-inclusive --cover-package=cloudinit \ +commands = pytest \ + --durations 10 \ + {posargs:--cov cloudinit --cov-branch \ Can you change this to `--cov=cloudinit` ? It makes it obvious that cloud-init is a value to the `--cov` key, rather than some other thing.

Yep, done.

[rjschwei]

Given that we are dropping Python 2 support with 20.1 release, I think the unittest target should be dropped rather than changing it.

[rjschwei]

Rather than making changes we should drop the sections for Py versions we no longer want to support.

[OddBloke]

On Sat, Feb 15, 2020 at 04:42:21AM -0800, Robert Schweikert wrote: > @@ -48,10 +46,10 @@ pyflakes3: @$(CWD)/tools/run-pyflakes3 unittest: clean_pyc - nosetests $(noseopts) tests/unittests cloudinit + python -m pytest -v tests/unittests cloudinit Given that we are dropping Python 2 support with 20.1 release, I think the `unittest` target should be dropped rather than changing it.

The `unittest` target is used in variables at the top of the file, which makes removing it more complex than simply deleting its definition. I intend to propose a separate, more complete cleanup of the Makefile in the near future.

> @@ -45,7 +44,7 @@ deps = -r{toxinidir}/test-requirements.txt [testenv:py26] deps = -r{toxinidir}/test-requirements.txt -commands = nosetests {posargs:tests/unittests cloudinit} +commands = {envpython} -m pytest {posargs:tests/unittests cloudinit} Rather than making changes we should drop the sections for Py versions we no longer want to support.

Agreed. As the tox targets aren't tied into anything else, I'll remove them cleanly as part of this PR.

[blackboxsw]

Generally I'm +1 on this, but when running tox -e xenial pytest in a python 3.7 environment the pytest run is failing for me on tox -e xenial target with 126 test errors due to python 3.7 raising StopIteration exceptions as RuntimeErrors.

We might have to find the right pinned version of pytest for the xenial tox env?

py3 env works fine with: pytest 5.3.5
xenial though doesn't have StopIteration exception handling support in pytest 2.8.7
I think we might need to specify pytest==3.0.7. As that version seems to have the fix needed https://docs.pytest.org/en/latest/changelog.html#id980

_______________ ERROR collecting cloudinit/tests/test_version.py _______________
.tox/xenial/lib/python3.7/site-packages/_pytest/runner.py:149: in __init__
    self.result = func()
.tox/xenial/lib/python3.7/site-packages/_pytest/main.py:431: in _memocollect
    return self._memoizedcall('_collected', lambda: list(self.collect()))
.tox/xenial/lib/python3.7/site-packages/_pytest/main.py:311: in _memoizedcall
    res = function()
.tox/xenial/lib/python3.7/site-packages/_pytest/main.py:431: in <lambda>
    return self._memoizedcall('_collected', lambda: list(self.collect()))
.tox/xenial/lib/python3.7/site-packages/_pytest/python.py:604: in collect
    return super(Module, self).collect()
.tox/xenial/lib/python3.7/site-packages/_pytest/python.py:458: in collect
    res = self.makeitem(name, obj)
.tox/xenial/lib/python3.7/site-packages/_pytest/python.py:470: in makeitem
    collector=self, name=name, obj=obj)
.tox/xenial/lib/python3.7/site-packages/_pytest/vendored_packages/pluggy.py:724: in __call__
    return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
.tox/xenial/lib/python3.7/site-packages/_pytest/vendored_packages/pluggy.py:338: in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
.tox/xenial/lib/python3.7/site-packages/_pytest/vendored_packages/pluggy.py:333: in <lambda>
    _MultiCall(methods, kwargs, hook.spec_opts).execute()
.tox/xenial/lib/python3.7/site-packages/_pytest/vendored_packages/pluggy.py:595: in execute
    return _wrapped_call(hook_impl.function(*args), self.execute)
.tox/xenial/lib/python3.7/site-packages/_pytest/vendored_packages/pluggy.py:249: in _wrapped_call
    wrap_controller.send(call_outcome)
E   RuntimeError: generator raised StopIteration

I think we may have to sort

[blackboxsw]

Xenial is busted for me: I think we need 3.0.7

[OddBloke]

We don't need or want 3.0.7, because that's not the version in xenial:

$ rmadison python-pytest
...
 python-pytest | 2.8.7-4        | xenial/universe         | all
...

The problem here is that, not especially surprisingly, the version of pytest in xenial doesn't work with a more recent version of Python. pytest 2.8.7 does work fine on xenial's version of Python, because we have this tox target in our Travis configuration, and the build is green for this PR: https://travis-ci.org/canonical/cloud-init/jobs/652102211

I think the "fix" here is to accept that we can no longer run the xenial tox env on focal. (This case has always been of limited utility because unless you're running xenial, you aren't actually testing a configuration that we ever expect to be deployed; no-one is running xenial's libraries with Python 3.8 in the real world.) As such, I don't think this is a great loss. I'll push up a change to the default tox targets that we run.

Does that sound OK?

[smoser]

Your argument above I think boils down to "Distro x has a different version of python than the developer's system. Therefore test of distro x in tox is of no use.". Taken to its limits, the argument basically defeats the entire intent of tox. Tox does not reproduce exactly the target of any environment, but usefully and easily approximates them.

The xenial tox environment was never a perfect environment. But it caught a very large number of errors that would only otherwise be exposed in much heavier test environment (a container or package build).

The value provided was less about testing a specific python but more about testing dependencies at the levels specified. Ie we have restricted our use of requests to version 2.9.1. If you used some feature of requests not present in that version, then test would fail. Tox did a good job of quickly finding failure where we used feature or behavior of a library at one version that was not available at a different version.

I don't have a good answer. But just dropping the environment will result in a higher number of developer failures being found by c-i that previously were found on developers run of 'tox'.

[OddBloke]

OK, I've just pushed up 8affaf2 which addresses this much better, I think. (For some reason, the PR hasn't updated, GitHub is having some Issues.)

[OddBloke]

On Tue, Feb 25, 2020 at 07:44:09AM -0800, Scott Moser wrote: Your argument above I think boils down to "Distro x has a different version of python than the developer's system. Therefore test of distro x in tox is of no use.".

I did _not_ say "no use", I said limited utility. And I'm right, the tests are of limited utility, precisely because they are an approximation of the target environment. The target isn't being removed, it's being taken out of the _default_ set of targets that a `tox` invocation with no arguments will run.

Taken to its limits, the argument basically defeats the entire intent of tox.

I didn't take the argument to its limits, intentionally, because that's not what I think.

Tox does not reproduce exactly the target of *any* environment, but usefully and easily approximates them.

Agreed.

The xenial tox environment was never a *perfect* environment. But it caught a very large number of errors that would only otherwise be exposed in much heavier test environment (a container or package build). The value provided was less about testing a specific *python* but more about testing dependencies at the levels specified. Ie we have restricted our use of `requests` to version 2.9.1. If you used some feature of requests not present in that version, then test would fail. Tox did a good job of quickly finding failure where we used feature or behavior of a library at one version that was not available at a different version. I don't have a good answer. But just dropping the environment will result in a higher number of developer failures being found by c-i that previously were found on developers run of 'tox'.

My concern is that if we relax the pytest requirement here, so that things pass on development machines, then CI will no longer catch use of _pytest_ features (or the existence of pytest bugs) which will cause package builds to fail. I'll spend a bit of time thinking about how we could work around this.

[smoser]

My concern is that if we relax the pytest requirement here, so that
things pass on development machines, then CI will no longer catch use of
pytest features (or the existence of pytest bugs) which will cause
package builds to fail. I'll spend a bit of time thinking about how we
could work around this.

I had assumed or implied that there was CI running that would test in an
actual xenial environment (run-container?) and would report failure.
I think that is a requirement either way. If you do not have that,
and xenial is removed from the default tox environment, then we're
basically guaranteed to see failure at daily-build-recipe time ...
way after the thing is merged.

Is there something that will run tests in xenial environment?

[OddBloke]

On Tue, Feb 25, 2020 at 08:14:26AM -0800, Scott Moser wrote: > My concern is that if we relax the pytest requirement here, so that > things pass on development machines, then CI will no longer catch use of > _pytest_ features (or the existence of pytest bugs) which will cause > package builds to fail. I'll spend a bit of time thinking about how we > could work around this. I had assumed or implied that there was CI running that would test in an *actual* xenial environment (run-container?) and would report failure. I think that is a requirement either way. If you do *not* have that, and xenial is removed from the default tox environment, then we're basically guaranteed to see failure at daily-build-recipe time ... way after the thing is merged. Is there something that will run tests in xenial environment?

This Travis job definition runs the "xenial" tox target on a xenial host using Python 3.5 (but with pip installed dependencies, _not_ the system's Python packages, and with Travis's Python 3.5 not the system one): https://github.com/canonical/cloud-init/blob/13e82554728b1cb524438163784e5b955c7c5ed0/.travis.yml#L46-L51 This Travis job definition runs a full xenial package build and cloud tests: https://github.com/canonical/cloud-init/blob/13e82554728b1cb524438163784e5b955c7c5ed0/.travis.yml#L20-L45 Both of these run in CI for each pull request, and in master for each merged commit. So: the latter job should definitely catch any failures we would see at daily-build-recipe time, and we have the former job to (a) fail faster, (b) fail with less confusing output, and (c) isolate any integration-level issues with the latter definition (e.g. if debootstrap fails because of archive issues, etc.).

[smoser]

i'm good with this at this point.
Thank you, Daniel.

[blackboxsw]

Thanks for the followup @OddBloke the xenial-dev makes a lot more sense to me.
I'm +1 on this, but I think we need to discuss when this needs to land as we have 2 FFes queued and potntially another bug-fix new-upstream-snapshot to handle as well during Ubuntu Focal feature freeze.

I want to make sure we are doing the least amount of cherry picking we can.

[blackboxsw]

The cloud-init project already has tooling to handle cherry-picks into specific Ubuntu series, so we decided we shouldn't be blocking landing any PRs in master due to external Ubuntu feature freeze constraints. As such, we can land this approved branch and upstream will sort Ubuntu-specific cherry-picks required during feature freeze.