[Tests] Add unit test for nvm_download #2406
Conversation
|
I'm confused; why is this a separate PR? A PR, once opened, is a permanent ref on a repo, so opening 2 PRs for the same change is highly undesirable (iow, between this and #2401, now both of them must stay open and be kept in sync manually if i want to avoid one of them dangling forever). Should I ignore #2401, and review this one instead? |
nmarghetti
force-pushed
the
install_other_repo_bis
branch
from
7f5a9cd to
6e479a5
Compare
|
Sorry for that, I did not realize it would cause this problem. But indeed, in the end only one of the 2 PRs should be discarded. |
|
PRs can't ever be removed; what I'd do is force push the same commit to both PRs before landing it. |
|
If one PR is closed without being integrated, is it causing any issue ? |
|
Yes. A closed and unmerged PR is a git ref that lasts for eternity, just dangling. |
|
Ah yep true... I can still use one of the PR to add another change to reduce the size of my other PR for Windows. Both PRs would be integrated at some point and not dangling then :D |
|
Do you have any preferred approach on this current change ? This PR with more compact code, or the other one ? |
install.sh
Outdated
| # Check if version is a tag | ||
| if [ "$(command git ls-remote "$(nvm_source "git")" "refs/tags/$NVM_VERSION" | wc -l)" != "0" ]; then | ||
| : | ||
| # Check if version is a ref | ||
| elif [ "$(command git ls-remote "$(nvm_source "git")" "$NVM_VERSION" | wc -l)" != "0" ]; then |
There was a problem hiding this comment.
why not command git ls-remote "$(nvm_source "git")" "${NVM_INSTALL_VERSION}" | grep? that would tell you if it was refs/tags/ or `refs/heads/.
There was a problem hiding this comment.
I dont like grep so much here, as you can the in my other PR failing, just adding nvm_grep makes the CI failing...
Also, on what would I grep ? grep "$NVM_VERSION" ? I am not sure about the difference with using wc -l then ?
There was a problem hiding this comment.
Ah sorry yes, did not get your comment well in fact. Yes in fact I could put them both together as there is no difference. It was just based on my other PR where I was doing a difference between a tag and a ref.
There was a problem hiding this comment.
It seems important for us to know the difference, but it seems like a single ls-remote call would provide it if we used grep or similar to filter the results.
|
I still have to check deeper what is feasible or not. With my other PR #2401 which passed, everything looked good. I integrated it in the other PR for Windows #2391 which allow me to have the CI testing with the SHA. But with this current PR it seems that there is some problem. I dont get it. |
install.sh
Outdated
| elif [ "$(command git ls-remote "$(nvm_source "git")" "$NVM_VERSION" | wc -l)" != "0" ]; then | ||
| : | ||
| # Check if version is an existing changeset | ||
| elif ! nvm_download -o /dev/null "$(nvm_source "script-nvm-exec")"; then |
There was a problem hiding this comment.
This check here based on downloading nvm-exec with a given changeset seems to fail while on my other PR for Windows it seems to have worked, otherwise the CI would not pass. I need to check deeper.
|
Sure, sounds good. |
nmarghetti
force-pushed
the
install_other_repo_bis
branch
2 times, most recently
from
e6a1b55 to
a7722f8
Compare
nmarghetti
changed the title
nmarghetti
force-pushed
the
install_other_repo_bis
branch
from
a7722f8 to
d6872ae
Compare
ljharb
force-pushed
the
install_other_repo_bis
branch
from
d6872ae to
589c237
Compare
Playing a bit more with git, I changed a bit the code compared to the other same PR, which is more compact but changing a bit the previous code though.
It still have the exact same test that pass and which I think cover rather well all the cases I could think of.