load_backend_if_not_loaded: test get_profile_storage is called just once

[sphuber]

Fixes #4868

Add a test for aiida.cmdline.utils.decorators.load_backend_if_not_loaded
to make sure that it calls Manager.get_profile_storage only when a
profile and its storage haven't already been loaded.

The most naive solution would be to simply load a profile and storage,
mock Manager.get_profile_storage to raise, and then check that calling
the decorator raises. However, this runs the risk that if the decorator
is ever change to not call Manager.get_profile_storage the test would
pass, but not for the right reasons. We have to make sure before testing
get_profile_storage is not called twice, that it is called once to
begin with.

This is accomplished by mocking the method first and checking that it
actually gets called, before loading the profile storage directly and
then calling the decorator again, whilst checking the mocked method was
not called again.

[ramirezfranciscof]

@sphuber the error on the tests seem to be related to get_profile_storage, but this is just a test you didn't modify the function here. Is the mock leaking out of the context??

[sphuber]

Indeed, it seems that the context is not respected, because after this test has run, other tests fail because they cannot retrieve the profile storage. Not sure yet how that is happening and how to fix it.

[ramirezfranciscof]

Indeed, it seems that the context is not respected, because after this test has run, other tests fail because they cannot retrieve the profile storage. Not sure yet how that is happening and how to fix it.

Mmm indeed quite weird...

Does doing it outside the context manager and calling monkeypatch.undo() work? (ref)

[sphuber]

Think there were two problems. I was monkeypatching the fixture manager instead of the class (which is now fixed), but the first context is not working. I have to call monkeypatch.undo after it. Funnily enough, I don't need to call it again at the end of the test. Not sure what is going on. But at least now it should be working.

[ramirezfranciscof]

All the code parts are great! I just have a few things to ask/propose/discuss on the supporting comments.

[ramirezfranciscof]

Mmm I find the wording "check that it only loads it once" a bit confusing (does it mean it could load it more than once in the same call?). I think it would be more precise to use something like:

Suggested change

	"""Test the :meth:`aiida.cmdline.utils.decorators.load_backend_if_not_loaded` calls load profile only once."""
	"""Test that the :meth:`aiida.cmdline.utils.decorators.load_backend_if_not_loaded` does not re-load the profile if it was already loaded."""

[sphuber]

Fair that it could be more precise but I think it should state then:

"""Test that :meth:`aiida.cmdline.utils.decorators.load_backend_if_not_loaded` calls `Manager.get_profile_storage` only once."""

At least that is the thing that we are actively testing.

[ramirezfranciscof]

I think this explanation is a bit less critical than explicitly stating why we are doing the first mock (I understand from the commit message, but perhaps it would also be good to have it in the code).

I would maybe have a comment before this one, on the first mock or in the docstring, saying something like:

# This test assumes the method uses ``get_profile_storage`` to load the profile, so we need to first check that this is the case.
# If this underlying fact changes, the test will need to be adapted.

Then this comment can be reduced to something like:

# The previous call was mocked and thus did not use the actual method, so we need to explicitly call it here to load the profile for the test.

[ramirezfranciscof]

Suggested change

	# This is necessary, despute the previous change being in a context. Without it, subsequent tests that require
	# the profile storage will fail.
	# This is necessary, despite the previous change being in a context. Without it, subsequent tests that require
	# the profile storage will fail.

I would maybe explicitly say that we don't currently understand why it is behaving this way and from what we know it shouldn't. Otherwise it reads a bit like this is just a reminder and there was a reason that should be deducible from it.

[ramirezfranciscof]

LGTM!