Mock get_platform / add CHARMHELPERS_IN_UNITTEST

[ajkavanagh]

Patch osplatform.get_platform() to return "ubuntu" if the environment
variable CHARMHELPERS_IN_UNITTEST is set to something truthy. This is to
enable unit testing in classic charms on NON ubuntu systems (e.g. Debian
bookworm) where the function is often called via a decorator that is
evaluated at module load time and thus is very, very tricky to mock out,
as it is often 'hit' during the test discovery phase.

[freyes]

Not pretty, but it gets the job done, and considering the scale at which we operate charm-helpers this approach is reasonable to me. I just left one comment inline.

[freyes]

the docs suggest that setting this env variable to "false" (considering that 'true' is valid to opt in), the doc should say something like "anything that python evaluates to true"

[samuelallan72]

@ajkavanagh @freyes is it only debian where this is a problem? Because I hit this today and was investigating - it seems that it's actually a bug in charm-helpers get_platform, where it's not correctly detecting debian. Or perhaps it's not supposed to support debian after all - the comment suggests perhaps it's a hack (where it guesses it's probably ubuntu even though python returned debian? but this leaves it open to false positive if it's actually running on debian).

Before we mock something like this, can we determine what the actual issue is, and what the intended behaviour on debian systems is? This kind of mock might introduce subtle issues if for example someone runs tests on centos.

For reference, I opened bug https://bugs.launchpad.net/charm-helpers/+bug/2040958 and patch #865

Also to add, if charm-helpers actually runs fine on debian (eg. debian bookworm in tests), why mock it to let it run in tests on debian, but crash with an error when someone actually tries to run it on debian? Can we simply detect debian properly all the time and load the ubuntu modules in that case?

[samuelallan72]

We should also make a pass over the rest of the conditions in get_platform too, depending on how we resolve this issue - several have comments about 'fails to run tests locally without this'.

Another way to approach this, which could be cleaner for everything, would be to have logic something like:

if centos based system:
  return "centos"
elif ubuntu based system:
  return "ubuntu"
else:
  warn("system %s is not supported, defaulting to loading ubuntu modules")
  return "ubuntu"

This avoids cases where users get unexpected runtime errors, avoids extra mocking, and avoids more environment variables or config for users running tests to have to worry about.

[ajkavanagh]

@samuelengineer yes, it's not detecting Debian bookworm due to it being "Debian" rather than "debian". We should probably throw a .lower() into the string to make detection case agnostic. However, it's also fundamentally broken as Debian doesn't use lsb_release and that also fails. Maybe we should just rip out Debian support (and not pretend it is Ubuntu) and either 'do it properly', or not at all. I favour the latter as charm-helpers is deprecated (sort of) due to the ops framework.

Also, I'm going to close this as we actually found a much simpler way of detailing with the issue; mocking the functions out in unit_tests/__init__.py and making them return a sensible default for the charm under test. This has the added benefit of further (and more properly) insulating the unit tests from the platform they are being run on. See https://review.opendev.org/c/openstack/charm-nova-compute-nvidia-vgpu/+/899177/2/unit_tests/__init__.py as an example.

[samuelallan72]

Ok, thanks for the clarification :)