Better default behaviour inside of unit tests

[pengwyn]

Occasionally there can be a race condition in tests which use the KV store. This is because the store does not need to be tolerant of concurrent writes in normal juju execution, but when tests are run in parallel they can write to the same (default) sqlite DB.

See https://bugs.launchpad.net/charm-helpers/+bug/1911919 for a collection of similar bugs.

The workaround in charms has been to manually create the KV store with ":memory:" so that each test will run with a different DB. This PR replicates this behaviour, by changing the default KV store location if zaza is imported. It can be overridden with the UNIT_STATE_DB as normal.

The actual fix is very small. The bulk of the PR is introducing a test to see that a) this behaviour can be forced and b) that the behaviour is fixed when a fake zaza is imported. I forced the problem by starting a subprocess that locks the DB.

My main worry with the PR is the crude checking of "zaza" in the imported modules. But I went with this approach to avoid any extra dependencies of charmhelpers on zaza or zaza on charmhelpers.

The tests can be checked by looking at commit ec50d90, which should give a failure.

[pengwyn]

After putting this together, I realise I have focused only on zaza. Does that capture most charm tests, or would a different detection of e.g. unittest be worthwhile?

[ajkavanagh]

I really like the intention of this change, and it's approach to having a default for 'testing mode'. However, I don't really like the 'linking' to zaza; i.e. creating it as a dependency. i.e. charms use charm-helpers; zaza tests charms (and, when incorrectly mocked), accidentally use charm-helpers, and in this case, expose that it's not multi-process/thread safe. This change kind of creates a reverse dependency of charm-helpers on zaza, (even if only tangentially).

My first thoughts on how to resolve this for consideration, are instead to add a charm-helpers specific environment variable that, if set to some value, will inform charmhelpers that it needs to use :memory: for the kv store, and then a concomitant change in zaza to use this as a default. Then the dependency change would be "zaza depends on charms depends on charm-helpers" and charm-helpers is unaware of what is saying "use :memory:". Thoughts?

[pengwyn]

Yes I see your point and I'm liking the module check less as I think about it more.

Writing out what might be good to have:

1. An external option for KV store to be concurrency safe.
2. A future proof way for the option to indicate "in testing mode"
3. No dependency on other packages
4. Not require the charm-writer (who doesn't know about the charm-helpers details) to explicitly set this option.
5. Don't cause surprise logic change with just an import.

It's 4 that is the tricky one, but I think your suggestion will handle that case too. The problem that people seem to be facing currently is 4, because you have to be bitten by the concurrency issue to know you should look for these changes. And we definitely don't want to break 5 as you mentioned. Finally, relying on UNIT_STATE_DB could break future store changes.

Here's a list of approaches I've thoought of, plus what you mentioned:

• A) The user must manually set UNIT_STATE_DB=:memory: and make sure it is passed to tests. No code changes. Ticks 1,3,5 but crucially misses 4.

• B) The user must manually set CHARM_HELPERS_TESTMODE=1. charm-helpers bases its default behaviour on this. Ticks 1,2,3,5 but still misses 4.

• C) As the original PR, autodetect zaza, but allow override with UNIT_STATE_DB. Solves 1,3,4, but fails 5.

• D) Detect if currently running in a test suite more rigorously. AFAIK this isn't possible with e.g. unittest. If so it'd solve 1-5

• E) As with B, but zaza also sets CHARM_HELPERS_TESTMODE on test run (not just import). Solves 1-5, although there could be problems if not using zaza.

• F) As with E, but zaza sets ZAZA_TESTS instead and charm-helpers then listens for that instead of an import.

Option E is my interpretation of what you wrote and it seems to tick the most boxes. I'm very new to charms, is the coverage of zaza across various charm packages close to 100%?

I added F too but that still suffers from the reverse dependency issue.

[pengwyn]

I make a small change to switch from checking for zaza to looking at CHARM_HELPERS_TESTMODE instead.

[pengwyn]

My bad! I think I got confused originally and didn't realise the test failures were all unit tests.

I like that idea, so I made an update to still keep CHARM_HELPERS_TESTMODE but the default is now "auto" which looks for one of those juju env vars.

It's a bit over-engineered, but this should give an option to anyone doing odd tests with faking JUJU_* envs to still use the in-memory version. I also added a test for this case.