aiidateam / aiida-core

The official repository for the AiiDA code
https://aiida-core.readthedocs.io
Other
434 stars 187 forks source link

pytest fixtures provided by aiida-core - what more should be included? #3473

Open ltalirz opened 5 years ago

ltalirz commented 5 years ago

aiida-core 1.0 now ships with a number of fixtures for running tests, but there is clearly room for improvement.

We should decide which of the following features should also be included:

fixtures by @sphuber in aiida-qe:

The automatic comparison files generation and checking is done through the pytest-regressions plugin which is added as a dependency. With this pytest plugin and the two fixtures, it is really easy to write and run tests for calculation jobs and parsers without actually having to run the calculations. Note of course, as mentioned before, that some tests that actually run the code will still be useful and desirable.

pointers from @greschd concerning aiida-pytest:

One thing that I’ve found to be helpful is having an option to wait for user input before wiping the test DB. That allows for inspecting the state better before it’s gone. See here: https://github.com/greschd/aiida_pytest/blob/migrate_v1/aiida_pytest/_configure.py#L152

Regarding the current fixtures in aiida-core:

greschd commented 5 years ago

Another thing I had forgot to mention: It would be great to print the AIIDA_PATH during setup -- again to allow inspecting the test DB.

I think pytest has some way to allow for printing before the tests start, but I haven't looked into that.

chrisjsewell commented 5 years ago

I think pytest has some way to allow for printing before the tests start

FYI the aiida-crystal17 conftest has some of the more advanced features of pytest, like printing a header (pytest_report_header), setting up some command-line and configuration file options, and running selective test sets.

greschd commented 5 years ago

This might require that we define the root_dir already at an earlier time (before the fixtures are being set up), and then pass it on to the TestManager. Shouldn't be a problem though - or is the root_dir supposed to be changeable between different tests?

ltalirz commented 5 years ago

or is the root_dir supposed to be changeable between different tests?

Haven't yet seen a use case for this, so not at the moment.

espenfl commented 5 years ago

Yes, so we should probably make something like a aiida_profile.root_dir or similar, so that we do not have to use the protected aiida_profile._manager.root_dir. Second, we introduced the posibility to pass arguments to pgtest (needed for instance to set the pg_ctl path if you have multiple Postgresql installations) in #2941 so we need to open for that again. Easiest I guess is just to make the fixture parameterized.

espenfl commented 5 years ago

Also, if there is a feature to test CalcJob I would think the extension to WorkChain should be rather straightforward? We have been running mock executables here: https://github.com/aiida-vasp/aiida-vasp/blob/develop/aiida_vasp/commands/mock_vasp.py which is triggered from a WorkChain test as: https://github.com/aiida-vasp/aiida-vasp/blob/develop/aiida_vasp/workchains/tests/test_vasp_wc.py

This is rather clean and easy. However, gives some problems during development when tracking down where things fail (the combination of commands in shell/script and quite a lot of files makes this a bit hard to track). In particular it gets a bit nasty when we for instance want to test something like a k-point convergence WorkChain, like here: https://github.com/aiida-vasp/aiida-vasp/blob/develop/aiida_vasp/workchains/tests/test_converge_wc.py where we need to pass an argument that gives an indication where to pull the files. I think there is no way around this as we need the files to perform realistic tests, but it would be nice to have a more formal context to do this. Also, in particular a context that makes it easier to eject sensible error messages when the test fails etc.

espenfl commented 5 years ago

Yes, so we should probably make something like a aiida_profile.root_dir or similar, so that we do not have to use the protected aiida_profile._manager.root_dir. Second, we introduced the posibility to pass arguments to pgtest (needed for instance to set the pg_ctl path if you have multiple Postgresql installations) in #2941 so we need to open for that again. Easiest I guess is just to make the fixture parameterized.

In order to reintroduce the pgtest argument passing, I have opened a WIP PR #3481 on this where we can discuss this issue in detail.

My vote going forward is to open for passing options to pgtest in a config file.