Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bugs in _store_from_cache and repository.erase #3777

Merged
merged 12 commits into from Feb 21, 2020

Conversation

greschd
Copy link
Member

@greschd greschd commented Feb 20, 2020

The _store_from_cache needed to erase the content of its sandbox folder before copying over the content of the cache source. Currently, the existing contents are mixed with with the contents
to be copied in.
In fixing this, we discovered an issue in repository.erase, where an unstored node would try erasing the (inexistent) folder in the permanent repository, instead of the SandboxFolder.

Fixed in collaboration with @sphuber, so this should be reviewed by someone else.

The _store_from_cache needed to erase the content of its sandbox
folder before copying over the content of the cache source.
Currently, the existing contents are mixed with with the contents
to be copied in.
In fixing this, we discovered an issue in repository.erase, where
an unstored node would try erasing the (inexistent) folder in the
permanent repository, instead of the SandboxFolder.
@greschd greschd requested a review from ltalirz February 20, 2020 14:55
@greschd
Copy link
Member Author

greschd commented Feb 20, 2020

I guess we should add some regression tests for this...

The regression test creates a Data node with a file in a directory
in the repository. After storing, it creates a clone, and stores
it when caching is enabled. Finally, the hashes of the original
and clone are compared.
Checked that this test fails when the store_from_cache fix is not
present.
@greschd
Copy link
Member Author

greschd commented Feb 20, 2020

@sphuber do you also want a regression test for the Repository.erase part?

@sphuber
Copy link
Contributor

sphuber commented Feb 20, 2020

@sphuber do you also want a regression test for the Repository.erase part?

Actually, a very simple one should not be too much work and probably give a lot of mileage. Just one where you store some files and call erase on unstored node and one where you call erase after storing (passing force=True). That should already cover quite a bit

@greschd
Copy link
Member Author

greschd commented Feb 20, 2020

@ltalirz in the test_store_from_cache (which is a new-style pytest test, yay) I now have to manually call reset_db, because some other part of the test suite (probably tests not using the fixtures) trashes the User.
Maybe it would make sense to also have a fixture which resets before the tests, not only after? It seems to be a valid pattern that you need the DB to be "pristine" for a certain test.

@greschd
Copy link
Member Author

greschd commented Feb 21, 2020

Docker check failing seems unrelated to this PR.

@greschd
Copy link
Member Author

greschd commented Feb 21, 2020

For the repository tests, I added 3 cases:

  • erase unstored node
  • erase stored node with force=True
  • erase stored node (no force flag), check that it raises

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @greschd looks good, just request to add small comment in the code

aiida/orm/nodes/node.py Show resolved Hide resolved
@greschd
Copy link
Member Author

greschd commented Feb 21, 2020

Changed it to implicitly rely on #3783. Failing on purpose right now, but it should start working again once that is merged and this branch is updated.

@sphuber sphuber merged commit 5e26bac into aiidateam:develop Feb 21, 2020
@sphuber sphuber deleted the fix_store_from_cache branch February 21, 2020 14:40
csadorf pushed a commit that referenced this pull request Mar 5, 2020
The `_store_from_cache` needed to erase the content of its sandbox
folder before copying over the content of the cache source. Currently,
the existing contents are mixed with with the contents to be copied in.

In fixing this, we discovered an issue in `Node.repository.erase`, where
an unstored node would try erasing the (non-existent) folder in the
permanent repository, instead of the `SandboxFolder`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants