aiidateam / aiida-testing

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

Archive cache fixtures #67

Closed janssenhenning closed 1 year ago

janssenhenning commented 1 year ago

Supersedes #56 (and thus also #29)

Sorry about the new PR, but I moved aiida-fleur to use the branch of #56 and did not want to rename the fixture names there until aiida-testing has a stable release of these fixtures. (AFAIK you can't change the source branch of a PR, right?)

The names of the fixtures were changed to more accurately reflect their purpose and agree with the current terminology in aiida-core

Flipped the logic of archive migration. Now done by default and only when the flag --archive-cache-forbid-migration is given it is not migrated. Additionally migration is done in a temporary directory to not modify the test files if not asked for explicitly

Removed run_with_cache fixture as it is the same as enable_archive_cache with only slightly different usage Added usage guide

ltalirz commented 1 year ago

thanks @janssenhenning and sorry for the delay on my side (very busy).

is there any part part of the code you'd like me to look at in particular?

are any of the comments from https://github.com/aiidateam/aiida-testing/pull/56 left unaddressed?

janssenhenning commented 1 year ago

thanks @janssenhenning and sorry for the delay on my side (very busy).

@ltalirz No problem

is there any part part of the code you'd like me to look at in particular?

are any of the comments from #56 left unaddressed?

Probably the most important part is, whether the logic of determining the filepaths is now understandable and well documented I also made one additional imrpovement here. When a archive already exists and during a test run it has to be migrated to the correct version, this is now done in a temporary directory to remove unnecessary noise in the git repository (if the overwrite=True option is not passed to enable_archive_cache) Would it make sense to add another commandline flag for the overwrite option to overwrite all archive caches?

ltalirz commented 1 year ago

Would it make sense to add another commandline flag for the overwrite option to overwrite all archive caches?

I think so, yes

ltalirz commented 1 year ago

Probably the most important part is, whether the logic of determining the filepaths is now understandable and well documented

That came out quite clearly in the docs, thanks!

janssenhenning commented 1 year ago

@ltalirz addressed all your comments and added --archive-cache-overwrite option to replace all archives in a test run

ltalirz commented 1 year ago

Thanks @janssenhenning !

It appears I can't commit the suggested changes myself. Please do so. Afterwards I will merge