Add more unit tests to "spack.relocate"#15654
Conversation
alalazo
force-pushed
the
maintainers/improve_relocate_coverage
branch
from
860c9af to
6b22e22
Compare
|
@gartung I'm curious of what you think of small PRs like this one as an additional way to work on improving coverage of @scheibelp Do you know if there's the possibility, when we'll eventually merge this PR, to give the authorship to Patrick? I don't mind being a co-author but I'd like that people looking at this part of the code sees him as the author (since I'm just doing trivial cleaning and unit testing of functions). |
scheibelp
left a comment
scheibelp
left a comment
There was a problem hiding this comment.
This looks great but I have a few requests relating to docstrings. Let me know if you agree or if the requests don't make sense (I had to think about this awhile to come up with suggestions that seemed good, so I wouldn't be surprised if I made it more confusing).
Also added parametrized unit tests for that function
Added parametrized unit tests for that function
The function doesn't set anything, but constructs and returns a placeholder of the correct length
alalazo
force-pushed
the
maintainers/improve_relocate_coverage
branch
from
f600b70 to
d86cee6
Compare
|
Thanks for the review @scheibelp Let me know if anything else is needed here. |
|
Thanks for the review @scheibelp ! |
This was an oversight in spack#15654 since `os.path.isfile` checks that the something is a file in the current filesystem.
This was an oversight in #15654 since `os.path.isfile` checks that the something is a file in the current filesystem.
3 participants
This PR introduces trivial refactoring in:
get_existing_elf_rpathsget_relative_elf_rpathsget_normalized_elf_rpathsset_placeholdermainly to be more consistent with practices used in other parts of the code and to simplify functions locally. It also adds or reworks unit tests for these functions and extends their docstrings.