ansible/ansible — pull request #1925 by jvantuyl (2013-01-27 02:38:45+00:00)
Title: restore unit tests erronously removed by bugfix

So, it looks like the fix in pull request #1913 made a few more changes than were necessary (or helpful).

It did one good thing: fixed the use of subprocess.call where a subprocess.Popen should have been used (this was actually a merge bug caused when I was rebasing it to keep up).

There was intentionally no test of this (i.e. integration tests). This was because I didn't want to make the test suite dependent on apt-key, since it would only pass on Debian. I figured that making the test suite Debian-specific was a no-no.

Rather than doing integration testing of apt-key, these are unit tests that verify the correct download / add / removal logic. Just for a sanity check, I checked out the version merged, and the tests passed (though, again, didn't test what failed).

To further cloud things, the tests used some methods that the committers / reviewers seemed to not recognize. Ansible tests have two complications that necessitated them:

Firstly, modules aren't imported in the same interpreter as the one doing the testing, so tools like Mock and such can't be used to patch functionality during the test.

Secondly, if you solve the first problem by manually mocking in the module, it's critical to prevent leaking any of the test code into production. In C, this is typically solved by using #ifdef. It's possible to do the same thing in Python by wrapping the test code in an "if" at a high level.

In this case, the reviewers fixed the bug (which was good), then misinterpreted the if statement as an indentation error. After "fixing that", they had leaked test code into the production code. Since they didn't appreciate what the tests were actually testing, they "fixed" this by removing the "useless" tests.

This commit puts the unit tests back in and I have verified that they pass (just as before). As before, critical application logic is now tested, no test code is defined or executed in production, and the previous bugfix is still present.

If an integration test is strongly desired, I can write one. Similarly, I'm very receptive to any suggestions on how to test this functionality without this ad-hoc mocking.