aiidateam / aiida-testing

A pytest plugin to simplify testing of AiiDA plugins.
MIT License
5 stars 6 forks source link

Add sanity check to see that 'aiida-mock-code' executable runs #13

Closed greschd closed 4 years ago

greschd commented 4 years ago

Add a "sanity check" session-scoped fixture (with autouse) that checks that the aiida-mock-codes executable works. For example, it could fail when pkg_resources detects an incompatibility in the requirements.

This probably also needs some change in the aiida-mock-codes executable itself, giving it a --sanity-check option or similar.

ltalirz commented 4 years ago

it could fail when pkg_resources detects an incompatibility in the requirements

This happened only on Github actions for me and cost me a lot of time to figure out...

Will add this fixture now.. still, I wonder whether it's not better to simply put aiida-mock-code into scripts. How is it sane that pip install goes through fine, but when you import pkg_resources you get an exception?

greschd commented 4 years ago

How is it sane that pip install goes through fine, but when you import pkg_resources you get an exception?

¯\(ツ)

AFAIK this is the behavior for all console_scripts entry points. Makes me wonder why we never encountered this issue with verdi.. maybe there is more to it.

ltalirz commented 4 years ago

Yes... what if you are working with a couple of plugins that have some conflicting dependencies?

Does that mean one should not be able to use aiida-mock-code then?

Makes me wonder why we never encountered this issue with verdi

Turns out verdi does not use load_entry_point

#!/Users/leopold/Applications/miniconda3/envs/aiida_rmq_py3/bin/python

import re
import sys

from aiida.cmdline.commands.cmd_verdi import verdi

if __name__ == '__main__':
    sys.argv[0] = re.sub(r'(-script\.pyw?|\.exe)?$', '', sys.argv[0])
    sys.exit(verdi())

And here the mock code

#!/Users/leopold/Applications/miniconda3/envs/nanoporous_pipeline/bin/python
# EASY-INSTALL-ENTRY-SCRIPT: 'aiida-testing','console_scripts','aiida-mock-code'
__requires__ = 'aiida-testing'
import re
import sys
from pkg_resources import load_entry_point

if __name__ == '__main__':
    sys.argv[0] = re.sub(r'(-script\.pyw?|\.exe)?$', '', sys.argv[0])
    sys.exit(
        load_entry_point('aiida-testing', 'console_scripts', 'aiida-mock-code')()
    )
ltalirz commented 4 years ago

I guess one would really need to look into the pip source code to figure this out. There is something on stackoverflow but it is actually not quite correct.

E.g. if I install aiida-core from the git repo locally, I also don't get the pkg_resources import.

It should somehow have to do with the pyproject.toml but if I copy the one from aiida-core to aiida-testing; uninstall and reinstall again, the console script still uses pkg_resources.

ltalirz commented 4 years ago

Hehe: here is the monkey

https://github.com/aiidateam/aiida-core/blob/0f069a70035337b880a0750f94adb0ada0d1f45f/utils/fastentrypoints.py#L36-L42

greschd commented 4 years ago

So, add from utils import fastentrypoints # pylint: disable=unused-import into setup.py, and also copy the utils/fastentrypoints.py into the source?

greschd commented 4 years ago

Extended instructions here: https://pypi.org/project/fastentrypoints/

ltalirz commented 4 years ago

will do!

greschd commented 4 years ago

Do you think we still need the sanity check, now that the most obvious failure mode is gone?

ltalirz commented 4 years ago

It's already in #33 and I'd suggest we keep it. The problem is that errors in aiida-mock-code are really hard to debug otherwise, the only way you notice is that the result of the calculation is not as expected.

ltalirz commented 4 years ago

Hm... actually the naive way of adding the option both broke the regular use of aiida-mock-code and, somehow, the exception raised in the autouse=True test fixture did not end up in the test results (even if I made it raise in general).

Well, for the time being I've removed the check again. Let's hope this never comes up again...

greschd commented 4 years ago

In principle I agree that the sanity check would be good to have.

I think I know what was going wrong with the "regular" use though: You can't turn aiida-mock-code into a click command, because we don't have control over which arguments will be passed to it from the _aiidasubmit.sh file.

So, the (somewhat ugly) way to do this would probably be if sys.argv[1] == '--sanity-check'. Not sure what went wrong with the pytest fixture, though.

greschd commented 4 years ago

That means if ever a plugin creates --sanity-check as the first flag to be passed to a code, that would then break the mock code..