Improve unit test coverage for v2/snap.py

[james-garner-canonical]

snap.py currently has pretty decent code coverage of unit tests at 85%. This PR raises coverage to 99%, using the same approaches to mocking external processes (e.g. snapd) that the existing tests used.

Only the following seemingly unreachable branch is not covered:

def __init__(self):
    if not self.snapd_installed:
        raise SnapError("snapd is not installed or not in /usr/bin") from None
    self._snap_client = SnapClient()
    self._snap_map = {}
    if self.snapd_installed:  # <-- unreachable False branch, since a SnapError is raised above
        self._load_available_snaps()
        self._load_installed_snaps()

(Perhaps a follow up PR can remove this seemingly redundant check.)

I've left in the pyright directives in test_snap.py that I added to silence warnings as I added tests. While pyright is not a dependency of this project, it seems to be the recommended type checker for ops and charms, and if this project considers moving towards more rigorously typed code in future these may be useful. The could be removed before merging this PR though.

[james-garner-canonical]

I'm not really sure what's going on with the failed integration test, but when running them on my machine they seemed a bit non-deterministic. Well, one relating to the terraform location (/usr/bin/terraform on my machine, when the tests want /usr/local/bin/terraform) seemed to consistently fail. But in general, others would occasionally fail, but typically pass, even when run on the unmodified main branch (including the test_snap_set_and_get_with_typed test that failed in this run).

EDIT: And rerunning the failing test (ubuntu integration test) on github lead to it passing ...

[benhoyt]

Are these tests actually useful?

[james-garner-canonical]

I've removed some of the overly specific tests and factored the remaining ones out into separate tests.

More generally, I've added some better documentation for the tests added in this PR.

If you think any of the remaining tests added in this PR are too specific and fragile, I can remove them. I don't think they're out of character with the already existing tests, but I don't think it would be worth adding them if they're just going to increase the maintenance burden.

[benhoyt]

For lack of a better place to comment: the main thing I'm wondering here is whether all of this test coverage is worth it. Tests are code, so they have a maintenance cost, and I'm wondering whether some of these pay for themselves. In addition, a lot of these tests rely quite a bit on patching and implementation details, which always makes things a bit more fragile.

Overall, I don't mind including some of these at least. Though I think our future efforts might be better spent in creating a better v3 API.

[benhoyt]

Looks reasonable to me now, thanks for your effort here.