Added unit test harness.

[petevg]

This is a first pass at a generic unit testing library for layered Juju charms. Feedback welcome, up to and including the name -- is "charms.unit" the right name?

@juju-solutions/bigdata @marcoceppi

[johnsca]

You could also encounter an IndexError here, and with the default value for mock_hookenv_status being True, this message is inaccurate.

My preference would be to have the attribute always exist and return None for self.last_status if the list is empty. That way, when you do self.assertEqual(self.last_status, 'my expected status') you'll get a reasonable (to me) message that None != 'my expected status' which is symmetric with the 'other status' != 'my expected status' result you'd get otherwise.

[johnsca]

Obviously, those should be tuples instead of just strings, but I think the point stands.

[petevg]

@johnsca Since the expected values are tuples, what I've found myself doing is things like:

self.assertEqual(self.last_status[0], 'active')

... which will raise an AttributeError if this routine returns None.

Maybe the best thing to do is to just seed the status list with (None, None) as its first element? That way, you always get a comparison that works.

I was originally raising the error because I didn't want people to waste time trying to figure out why their code wasn't setting a status, when the problem was that hookenv isn't actually mocked. You do have to deliberately turn if off, though, so maybe that isn't something that we need to protect people from ...

[johnsca]

I can see the argument for making the "you turned off mocking but still tried to fetch a status" error more obvious, but the message would be slightly misleading in that case. Even so, if you did leave mocking enabled and there was no status set, you'd get an IndexError no matter how you structured your assert (and actually, if you used self.assertEqual(self.last_status[0], 'active') instead of self.assertEqual(self.last_status, ('active', 'foo')) then the IndexError would be misleading because you might assume it had to do with your explicit index instead of the one inside the last_status property).

Whether we handle that by pre-populating with (None, None) or just adding an if condition to the property, I do think that last_status with no status having been set should return (None, None). Actually, I do prefer the if condition, though, in case you want to do an assert on the full list of statuses instead of just the last one. E.g.:

self.assertEqual(self.statuses, [
    ('maintenance', 'installing'),
    ('blocked', 'missing relation'),
    ('active', 'ready'),
])

[petevg]

@johnsca That's a good point. Being able to check against the whole status list is compelling, and my error didn't cover the case where the status is mocked, but not set yet.

Moved the setting of self.statuses into the init so that the property always exists, and added a conditional to return (None, None) if the list is empty.

[johnsca]

This seems wrong. You claim that you're only patching hookenv.status but you're actually patching away the entire hookenv library. It also doesn't work if status_set is imported into the module directly.

Also, your mock doesn't ever append anything into self.statuses.

[johnsca]

My preference here would be to use patch('charmhelpers.core.hookenv.status_set') so that you only have to do that once and it's good for anything that has imported the entire hookenv, and then look for and conditionally patch (or patch with create=True) status_set in each of self.local_modules to catch those cases.

[petevg]

@johnsca Ack! I neglected to port the line of code over that sets the statuses. Will fix that shortly.

I think that patching status_set directly makes sense. Our bigtop layer does just import hookenv, though, and that's a common pattern throughout our code. :-/ Let me think about it a bit -- maybe the right thing to do is to accept a string (or strings) that specify how we should patch out hookenv.

[johnsca]

Yeah, my habit is to import the entire hookenv so that pattern is common in the Bigtop charms but not as common across charms in general, particularly for status_set (e.g., see juju-solutions/charms.reactive#76). But my suggestion should cover both cases and doesn't end up mocking out other things from hookenv when it's only documented to mock out status_set.

[johnsca]

If they don't have a .py extension, I don't think you'd be able to import or patch them.

[johnsca]

The recursive support was added in 3.5 and won't work on trusty (which has 3.4).

[petevg]

johnsca: Yarr! We don't actually need it here, anyway (I was originally passing in recursive to try to catch everything in one fell swoop, but there were other issues with that picking up libs that I didn't want to patch). Will fix.

[johnsca]

This recursive form would be useful for, e.g., the OpenStack charms which have the unit tests at a higher layer than the layer itself in the repo, but wouldn't actually handle nested directories under reactive/ or lib/charms/layer/.

[johnsca]

I was confused about your comment about automatic mocking, but I still don't understand how this is supposed to work. This seems like it would catch from charms import layer; something_something(layer.options('foo')) but that's it. The docstring implies that you would detect and automatically handle something like from charms.layer.base import BaseThing.

[petevg]

@johnsca You're right. The user can always pass in some additional things to 'to_mock' in order to patch out other things, but we really want it to be automatic.

The solution that immediately springs to mind is to go back to patching out builtins.__import__ -- it's a lot easier to intercept things there, and you also skip walking the directory tree, missing non .py files, etc.

I think that I'm the only fan of patching __import__, though. Let me sleep on it, and see if I can cook up a better solution ...

(The solution is probably to actually read the files and parse the import strings; I'm concerned about the performance impact of that, though.)

[petevg]

@johnsca Following up on this: the fact that the charms.layers directory exists in layer-apache-bigtop-base has been protecting us from some of the trickiness inherent in mocking out the imports. It's not as simple as doing mock.patch('some.module.path', create=True), as that doesn't actually put mocks into the right places in sys.modules.

We can patch a mock into sys.modules without clobbering other things with a invocation like sys.modules['charms.layers.foo' = mock.Mock()]. I'm working making that happen automatically without intercepting __import__, or parsing the import lines of the files that we're testing.

[petevg]

Note: this README is really out of date. Will fix.

In the meantime, the docstring in the Harness class has up-to-date info on how this all works.

[marcoceppi]

I don't think there's any reason to keep this kicking around, it's version 0, landing now and using it to get better feedback is the best way forward. Great work! ⛵️