zincware / MDSuite

A post-processing engine for particle simulations
https://mdsuite.readthedocs.io/
Eclipse Public License 2.0
36 stars 7 forks source link

Low memory tests #565

Closed SamTov closed 1 year ago

SamTov commented 1 year ago
PythonFZ commented 1 year ago

I agree with @PythonFZ that the tests should not be duplicated. Context managers are nice (albeit a bit complicated), something like

def test_stuff():
    do_actual_test()
    mds.set_memory(1mb)
    do_actual_test()

probably also does the trick

The reason for the context manager was, that with mds.set_memory(1mb) we change a global MDSuite setting, so even if the test fails we need to make sure that we revert that change. I don't know if we can be certain that this happens if we don't use a context manager. This would not affect the CI, because here all tests have to pass but for local testing it might cause unexpected behaviour when running tests with pytest -n auto and such a test fails.

SamTov commented 1 year ago

I agree with @PythonFZ that the tests should not be duplicated. Context managers are nice (albeit a bit complicated), something like

def test_stuff():
    do_actual_test()
    mds.set_memory(1mb)
    do_actual_test()

probably also does the trick

The reason for the context manager was, that with mds.set_memory(1mb) we change a global MDSuite setting, so even if the test fails we need to make sure that we revert that change. I don't know if we can be certain that this happens if we don't use a context manager. This would not affect the CI, because here all tests have to pass but for local testing it might cause unexpected behaviour when running tests with pytest -n auto and such a test fails.

I like the idea but if you could write them or at least some it would be helpful as I am not sure of the best way to do it. Also, if you do that, see if you can replicate the CI error locally. It says that time isn't in the data that is loaded but this is not true...

SamTov commented 1 year ago

@PythonFZ Thanks for moving all the calculators that's awesome. Looks a lot nicer now. I realise the issue is that the data is loaded and then popped once and then we try it again. I will fix it now.