Added a section on best pratices for unit testing #1317

Merged
merged 2 commits into from Sep 21, 2016

Conversation

Projects
None yet
7 participants
Contributor

petevg commented Aug 30, 2016

Unit testing a layered charm can be a little tricky: you'd generally
like to run your tests without waiting for the charm to build, but
you'll run into ImportErrors if you do so, as you might be importing
modules from a layer that hasn't been added to your local environment,
prior to the build.

Added some notes on what we currently consider to be the best way to
work around these errors, as well as some notes on separating your unit
test config from your integration testing config.

Added a section on best pratices for unit testing
Unit testing a layered charm can be a little tricky: you'd generally
like to run your tests without waiting for the charm to build, but
you'll run into ImportErrors if you do so, as you might be importing
modules from a layer that hasn't been added to your local environment,
prior to the build.

Added some notes on what we currently consider to be the best way to
work around these errors, as well as some notes on separating your unit
test config from your integration testing config.
src/en/developer-testing.md
+
+For layered charms, it is often desirable to be able to run some tests before the charm has been built, however. For example, you may wish to run unit tests as you write your code. Waiting for the charm to build so that amulet can run tests on it would introduce unnecessary delays into the unit testing cycle. What follows are some best practices for writing unit tests for a layered charm.
+
+#### Create a separate tox ini file
@marcoceppi

marcoceppi Aug 30, 2016

Owner

This is excessively indented, suggest: ###

src/en/developer-testing.md
+[testenv]
+commands = nosetests -v --nocapture tests/unit
+deps =
+ -r{toxinidir}/unit_test_requirements.txt
@marcoceppi

marcoceppi Aug 30, 2016

Owner

I'd rather we do something like tests/requirements.txt rather than two requirements files in root

@petevg

petevg Aug 30, 2016

Contributor

That sounds good to me. I'll update the example.

src/en/developer-testing.md
+ -r{toxinidir}/wheelhouse.txt
+
+setenv =
+ PYTHONPATH={toxinidir}/reactive:{toxinidir}/lib
@marcoceppi

marcoceppi Aug 30, 2016

Owner

this can actually cause some collisions. I've found that removing reactive from PYTHONPATH and just doing from reactive import $layer works for unit tests since it's all python3

An example of breakage, layer:apt and the Python APT module, if both are present you'll never be able to import apt Python module

@petevg

petevg Aug 30, 2016

Contributor

Good point. I'll strip out the reactive bit.

Changed based on PR comments.
Fixed headline indenting, changed up the recommended .ini file setup a bit.

I'd really prefer the charm unit testing story to move away from mocks altogether, and use some standard stubs instead that act on boundaries, only when strictly necessary (e.g. network). The rational is: "if running real code is cheap, don't mock it". This is usually gives far more confidence that the difference components play well together, without having to run integration tests, which also have the problem that they hardly exercise all code paths (and that's not their goal, it's unit-test land).

A good old take from Fowler on the subject:

http://martinfowler.com/articles/mocksArentStubs.html

So to put it short, I'm -1 on the best practices proposed in this branch, and the only way I see to get rid of mocking is to actually make external layer code available when you run unit tests. After all downloading the relevant layer code is the same as downloading any external library with pip: it's a price you could pay just once, when you first run unit tests.

All in all, I'd rather like to see a helper that lets you download such external layers code (perhaps based on charm build).

I am +1 on what @freeekanayaka proposes but I am also +1 for this addition.

@freeekanayaka points to an excellent approach we should investigate. But I also feel we should document the current options we have for testing and revise the documentation when we come up with a better approach.

+
+For layered charms, it is often desirable to be able to run some tests before the charm has been built, however. For example, you may wish to run unit tests as you write your code. Waiting for the charm to build so that amulet can run tests on it would introduce unnecessary delays into the unit testing cycle. What follows are some best practices for writing unit tests for a layered charm.
+
+### Create a separate tox ini file
@chuckbutler

chuckbutler Aug 31, 2016

Collaborator

👍 FINALLY! Tox is mentioned in the testing story of our docs. Big +1 to this already

Collaborator

chuckbutler commented Aug 31, 2016

This works with how i'm currently writing charms. Abstracting a lob of libs into python modules like charms.docker.

This is a pretty high quality submission for a first draft of unit-testing charms. Thanks for the effort put in here @petevg

👍 LGTM

Owner

marcoceppi commented Aug 31, 2016

@freeekanayaka You make a good point, and potentially having a charm test (well, we have that today, but refactoring a bit) where it builds all layers then runs unit testing across the compiled unit would be a way to work around that.

We have some layers that try to strip out the unit_tests or tests directories on build. If we formalized something

Charm build will strip out the tests directory for you

Or if we had a layer.yaml option which allowed authors to say "here is my unit tests directory" we could get something like charm build --with-tests where we'd compile the layers and then execute the testing against it including the bits, where by default charm build wouldn't include tests.

Or, charm build --no-test which means we'd always compile the charm and test (then remove the directories specified as unit tests) unless the --no-test flag was passed.

You bring up some great points, I'd like to continue this discussion where the charm build tooling lives: https://github.com/juju/charm-tools

@marcoceppi the charm build/charm test approach sounds fine, as long as it takes advantage of local caching and doesn't take more than one or two seconds (say) for runs after the first one. This is critical in order to do effective TDD (you modify some code and want to get unit tests completed very quickly).

Agreed that it should probably be something living in charm-tools.

Contributor

petevg commented Aug 31, 2016

Thanks for your feedback, everyone!

@freeekanayaka: I actually floated the idea of adding a "--light" flag to "charm build" with the big data team. The idea was that the flag would tell charm build to just re-use remote packages if it had already fetched them.

The downside is that it's a little weird to have Python unit tests executing in a different directory than your source code. And you still do have some delay in test execution as local files are copied to the right place.

Additionally, we have people asking about unit testing now, and an alternate path in charm build is still x days/weeks/months in the future :-)

Does it make sense to merge this for now, and update with instructions on using charm build if/when we decide to add unit testing friendly functionality to it (or to charm tools)?

Collaborator

chuckbutler commented Aug 31, 2016

I am +1 to landing as is for now, as it provides a gateway to having better code coverage in charm layers. We can extend the doc as the new features land and are fleshed out. Until then its just painting the 🚲 🏠

@petevg: I agree with the downsides you mentioned (weirdness in executing tests in a different directory, extra time/complexity etc). Bonus points if those downsides could be avoided, however I'm not entirely sure how, given the current mechanics of layer composition. Perhaps layers should just be regular python packages, as it was somehow mentioned elsewhere (not sure what the downsides of that would be tho).

We do some very similar things in the openstack charms team. Essentially, we have charms.openstack where 'most' of the support and boilerplate code lives, and then 'just' the unique charm code goes into the layers.

We've arranged charms.openstack in a way that means we don't need to mock much of it out to get unit tests to run. However, we DO have to mock a lot of charmhelpers out to be able to do unit tests, and this is almost at the stage where you wonder what you are actually testing.

+1 to the idea of extending the 'test' to build the charm so that unit tests can calls on other layers' modules (although this begins to beg the question of whether layers should be 'unit testable' by themselves). However, this raises an 'elephant in the room' type problem in terms of 'repeatability' -- without the ability to version control the dependencies in layers, we can't get repeatable builds which means that any unit test run is only valuable at that point in time if the unit tests have to run with a built charm; there are pros and cons with this (and it's contentious).

@ajkavanagh: re mocking explosion, this what I meant above when mentioning the approach of faking the boundaries and the underlying "model" could help:

https://github.com/freeekanayaka/charmfixture

I plan to add support for other hook tools and parts of the backend, I believe it should then be relatively easy and convenient to simulate even complex interactions/conversations in relation hooks.

I'll try to demonstrate how the approach works in practice in the jenkins charm I've been working on (which already uses this technique, but not the library above).

@evilnick evilnick merged commit db12b80 into juju:master Sep 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment