search

lsmo-epfl / aiida-lsmo

AiiDA workflows for the LSMO laboratory at EPFL
Other
9 stars 13 forks source link

Add tests #34

Closed ltalirz closed 4 years ago

ltalirz commented 4 years ago

fix #14

This PR adds a first complete integration test of the Isotherm workchain.

It uses the aiida-testing package to mock the Raspa and zeo++ executables, meaning test time is spent only on the aiida-internal logic.

It also switches from Travis CI to Github actions and adds test coverage.

For review: All files in tests/data can be ignored.

ltalirz commented 4 years ago

I believe the execution time is never a problem since it can always be shortened.

I don't think this is true in general. I can certainly imagine examples where it becomes difficult to construct tests that run very quickly - in particular if you want to make sure that your workchain works for different classes of structures and when testing workchains with many steps. Using mock_code allows you to test the workchain using realistic use cases.

The underlying point here is that we would like to test the workchain logic and not the execution of the code binaries.

How easy would it be to maintain that, if code developers would introduce a change in the format?

I can see two main cases, in which you need to update the test database:

  1. If you change the inputs to one of your calculations
  2. If the plugin you are using (e.g. aiida-cp2k) drops support for the code version you were using (e.g. cp2k-7.1)

In that case, you can simply clear the test database and run the tests once locally, regenerating it from scratch.

Overall, I agree that we need to see how easy it is to maintain - this is our first time using the method, and perhaps it turns out that we need to update the test database more often than we would like to.

Given that this PR increases test coverage from 0% to 36%, I think it's worth giving it a try.

yakutovicha commented 4 years ago

Given that this PR increases test coverage from 0% to 36%, I think it's worth giving it a try.

Well, to me is not the main reason, there are many ways to increase the coverage :).

I don't want to block you from trying out this approach. My guess is that it will be more difficult to maintain compared to running executables as usual. Especially in this particular case. But maybe I am wrong.