Improve configurability of consumers and CLI, add tests
ClosedPublic

Authored by adamwill on Feb 8 2017, 5:08 AM.

Details

Summary

This is mainly about making the fedmsg consumers configurable.
We also do a bunch of cleaning up in the config settings and
offer more settings in the CLI as well.

I took the opportunity to rework the consumer class naming to
be more consistent; note this will break existing consumer
configs, they will need to be updated. I will update the infra
deployments.

We also add, well, tests. Lots of tests! All the consumers and
the entire results module are tested, which should help us be
confident these changes are OK. I didn't have enough time to
write tests for schedule or cli yet, will do it ASAP.

There's a little bit of general cleanup too - unused imports and
so on.

Test Plan

Run the tests! There are tests now. Also actually use
the CLI and ideally the consumers and make sure they really do
work, and my tests aren't lies.

Diff Detail

Repository
rOPENQA fedora_openqa
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
adamwill retitled this revision from to Improve configurability of consumers and CLI, add tests.Feb 8 2017, 5:08 AM
adamwill updated this object.
adamwill edited the test plan for this revision. (Show Details)
adamwill added reviewers: jsedlak, jskladan.
adamwill updated this revision to Diff 2854.Feb 8 2017, 5:12 AM

Fixing copyright blocks (whoops, copy/paste fail)

adamwill updated this revision to Diff 2859.Feb 9 2017, 8:01 AM

Improve test approach using fixtures

This makes the tests from the previous version a lot cleaner
(I think, at least). I finally figured out the riddle of how
to mock Wiki and OpenQA_Client the way I wanted. With that,
and some judicious use of test fixtures that yield bits of
themselves for the test to modify / inspect, I think this is a
lot better. We're not duplicating ugly 'constants' from test
file to test file, or modifying single copies of dicts over and
over.

adamwill updated this revision to Diff 2860.Feb 9 2017, 8:02 AM

Tiny tweak.

adamwill updated this revision to Diff 2861.Feb 9 2017, 8:04 AM

Start work on CLI tests

I started this afternoon, but got a bit distracted by cleaning
up the rest of the test suite. Still, here's where I got to.

Oh, one thing I should mention - to run the tests use python setup.py test or PYTHONPATH=./ py.test. Making plain py.test work is...not straightforward (there's a whole world of detail there, but yeah).

adamwill updated this revision to Diff 2862.Feb 9 2017, 9:01 AM

Use autospec when mocking

It makes the tests more likely to catch when APIs change.

adamwill updated this revision to Diff 2869.Feb 9 2017, 4:07 PM

Use mock.patch.dict to temporarily modify dicts

Where useful. In some cases we're just changing the same value
throughout the test, so it's not really necessary.

adamwill updated this revision to Diff 2870.Feb 9 2017, 4:53 PM

Document consumer tests better, fix test_configs

The parametrization stuff is a bit advanced so I figured it's
a good idea to explain it. While doing so, noticed test_configs
was broken, it wasn't actually checking all combinations, only
one consumer from each iterable.

adamwill updated this revision to Diff 2871.Feb 9 2017, 5:21 PM

Optimize the test consumer init process a bit

adamwill updated this revision to Diff 2872.Feb 10 2017, 12:49 AM

Complete test suite, lint test suite, few other cleanups

So now we go from 0% coverage to 91% coverage in one commit :)
Plus a few other little things like updating .gitignore and
stuff.

Some of the tests are very literal 're-implement the function'
tests, which I kinda hate, but I at least was careful to use
mock autospec wherever possible so the tests will break if the
mocked object API breaks. I think this is decent for a first
cut.

My reviews for DRs this long are going to be reduced to "Does it work on staging?".

Macro devops: I'll just probably ACK it and call it a day

Code looks alright, but I still have to review tests before ACKing.

Heh :) A lot of the length is just a data file, and a lot of the rest of the length is just the tests. I didn't really need to write *all* the tests, strictly speaking, but I did want to cover all the bits I was changing in the consumers, and that alone would've required quite a lot of what I wrote.

The actual code *changes* are mainly just the overhaul of the consumers, plus some tweaks to the logic of the CLI commands to make them fit in better and avoid the weird inconsistencies we noted in config setting use and such.

jsedlak accepted this revision.Feb 14 2017, 1:00 PM

I had to install py.test from pypi for test to be able to yield. It showed me this warning:

WC1 None pytest_funcarg__cov: declaring fixtures using "pytest_funcarg__" prefix is deprecated and scheduled to be removed in pytest 4.0.  Please remove the prefix and use the @pytest.fixture decorator instead.

but otherwise it looks OK.

This revision is now accepted and ready to land.Feb 14 2017, 1:00 PM
adamwill added a comment.EditedFeb 14 2017, 4:42 PM

Huh. What distro are you running, and what pytest version? I remember the yield stuff not being in the version in RHEL 6, but I thought it was in everything since (RHEL 7 plus all supported Fedoras).

There is a way to accomplish the same thing without yielding (you essentially declare a callback function to be run when the fixture is torn down) but it's ugly and I'd rather avoid using it unless really necessary.

edit: Oh wait! I remember. Up until pytest 3.0 you had to explicitly mark the fixture as one which used the 'yield' feature. I'll adapt the code to do that, as it's easy.

Closed by commit rOPENQAeab29fdc8487: Improve configurability of consumers and CLI, add tests (authored by Adam Williamson <awilliam@redhat.com>). · Explain WhyFeb 14 2017, 5:25 PM
This revision was automatically updated to reflect the committed changes.