Enable use of the caplog fixture in pytest tests, and add a cc_final_message test using it#461
Conversation
This package provides the `caplog` fixture which was later merged into pytest itself; from bionic onwards we can depend on its presence in the pytest package itself.
And call `bddeb` appropriately in the Travis integration test.
OddBloke
force-pushed
the
cc_final_message
branch
from
9a21304 to
98d5183
Compare
|
This additional |
|
| if templ_data['debian_release'] == 'xenial': | ||
| requires.append('python3-pytest-catchlog') |
There was a problem hiding this comment.
@smoser I don't know if you have a better idea of how to introduce a release-specific requirement. I looked at expanding read-dependencies to handle it, but it doesn't currently have any concept of release-specific requirements (and doesn't even take a release parameter).
There was a problem hiding this comment.
@OddBloke yeah I had thought initially of extending packages/pkg-deps.json (which ./tools/read-dependencies uses to differentiate pkgs per distro). We almost went down that route with a PR to support centos6 vs 7 pkg name differences (prior to us dropping py2 support).
As it is, we have a fractured set of dependency definitions:
- tox.ini defined series-specific dependency diffs that are not reflected in test-requirements.txt,
- .travis.yml uses
apt-get build-depto get package deps. - make ci-deps-ubuntu uses ./tools/read-dependencies directly, which means it will have no visibility to this xenial-specific requirement,
It'd be nice to put that release param into read-dependencies explicitly and let it source both test-requirements.txt and and specialized test-requirements-xenial.txt file if present. Then tox could call out that specific dependency in a separate file and read-dependencies could also source it if provided with a --release param.
I realize this may be more work than it's worth for this effort (and single use case). So I'm happy to be "talked off the ledge"
There was a problem hiding this comment.
@OddBloke yeah I had thought initially of extending packages/pkg-deps.json (which ./tools/read-dependencies uses to differentiate pkgs per distro). We almost went down that route with a PR to support centos6 vs 7 pkg name differences (prior to us dropping py2 support).
As it is, we have a fractured set of dependency definitions:
* tox.ini defined series-specific dependency diffs that are not reflected in test-requirements.txt, * .travis.yml uses `apt-get build-dep` to get package deps.
This is no longer the case as of 2be1223.
* make ci-deps-ubuntu uses ./tools/read-dependencies directly, which means it will have no visibility to this xenial-specific requirement,
Does anything use this? The only references to it I can find are its definition in the Makefile, and a Changelog entry.
It'd be nice to put that release param into read-dependencies explicitly and let it source both test-requirements.txt and and specialized test-requirements-xenial.txt file if present. Then tox could call out that specific dependency in a separate file and read-dependencies could also source it if provided with a --release param.
Agreed that we could certainly improve this.
I realize this may be more work than it's worth for this effort (and single use case). So I'm happy to be "talked off the ledge"
Yeah, given we only have a single motivating example currently makes this feel like more work than is justified: bear in mind that we will never add new Depends in already-released Ubuntu versions, so the only time we would see another case like this is (a) if we introduce a new build dependency, and (b) that build dependency is either named differently or absent in a target Ubuntu release. So this feels like a YAGNI situation, to me.
What do you think?
There was a problem hiding this comment.
- make ci-deps-ubuntu: is probably something that only I use, I can't find it in any qa-scripts or server-jenkins-logs. We might as well drop it (separate PR)
- As for improving test-requirements.txt to add seperate test-requirements-xenial.txt we can chat about that as a separate PR idea as well. I don't feel this case is strong enough to warrant it on this PR
I think you are right that this may be over-engineering by trying to extend read-dependencies to handle this single case.
We can revisit one we have one or two other use-cases which would require that change.
TheRealFalcon
left a comment
TheRealFalcon
left a comment
There was a problem hiding this comment.
Nice find of python3-pytest-catchlog
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
Thanks for the discussion and responses. My review comments were not blocking.
3 participants
caplogis only available inpytestitself from 3.0 onwards. In xenial, we only have pytest 2.8.7. However, in xenial we do havepytest-catchlogavailable (aspython3-pytest-catchlog), so we use that where appropriate.