Fix bug in aiida.engine.daemon.execmanager.retrieve_files_from_list
#4275
Conversation
sphuber
force-pushed
the
fix/4273/retrieve-files-from-list
branch
2 times, most recently
from
2e28699
to
b3926f2
Compare
The `retrieve_files_from_list` would loop over the instructions of the
`retrieve_list` attribute of a calculation job and for each entry define
the variables `remote_names` and `local_names` which contain the
filenames of the remote files that are to be retrieved and with what
name locally.
However, for the code path where the element of `retrieve_list` is a
list or tuple and the first element contains no wildcard characters, the
`remote_names` variable is not defined, meaning that the value of the
previous iteration would be used. This was never detected because this
code path was not actually tested.
This bug would only affect `CalcJob`s that specified a `retrieve_list`
that contained an entry of the form:
['some/path', 'some/path', 0]
where the entry is a list and the first element does not contain a
wildcard.
sphuber
force-pushed
the
fix/4273/retrieve-files-from-list
branch
from
b3926f2
to
135caf9
Compare
Codecov Report
@@ Coverage Diff @@
## develop #4275 +/- ##
===========================================
+ Coverage 79.18% 79.19% +0.02%
===========================================
Files 468 468
Lines 34476 34477 +1
===========================================
+ Hits 27295 27301 +6
+ Misses 7181 7176 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## develop #4275 +/- ##
===========================================
+ Coverage 79.18% 79.19% +0.02%
===========================================
Files 468 468
Lines 34475 34476 +1
===========================================
+ Hits 27294 27300 +6
+ Misses 7181 7176 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Since this creates the tests for this function, shouldn't we maybe add checks to make sure that you can (1) change the name of the file/folder while copying and (2) create dirs in the target directory (i.e. |
It's true that I only tested a small percentage of the pathways of this code, specifically the part that concerned the bug I am fixing there. Parts of it are tested indirectly through the integration tests that actually run some entire |
|
Yes, I agree it might not make sense to use this bugfix PR to try design the perfect coverage test for this function. I just thougth these two extra tests were particularly related to the functionality being fixed and might be worth including. Anyways, its up to you, I will aprove the PR so you can merge it if you prefer to leave these for later (or you can ping me to take a look if you decide to include them). |
|
Thanks @ramirezfranciscof . I think I will leave it as is for now. If another bug crops up, the test can be expanded |
Fixes #4273
The
retrieve_files_from_listwould loop over the instructions of theretrieve_listattribute of a calculation job and for each entry definethe variables
remote_namesandlocal_nameswhich contain thefilenames of the remote files that are to be retrieved and with what
name locally.
However, for the code path where the element of
retrieve_listis alist or tuple and the first element contains no wildcard characters, the
remote_namesvariable is not defined, meaning that the value of theprevious iteration would be used. This was never detected because this
code path was not actually tested.
This bug would only affect
CalcJobs that specified aretrieve_listthat contained an entry of the form:
where the entry is a list and the first element does not contain a
wildcard.