[tests] remove double-substitution in assert_ok and assert_not_ok by enlightened88 · Pull Request #3826 · nvm-sh/nvm · GitHub
Skip to content

[tests] remove double-substitution in assert_ok and assert_not_ok - #3826

Merged
ljharb merged 1 commit into
nvm-sh:masterfrom
enlightened88:patch-2
Apr 24, 2026
Merged

[tests] remove double-substitution in assert_ok and assert_not_ok#3826
ljharb merged 1 commit into
nvm-sh:masterfrom
enlightened88:patch-2

Conversation

@enlightened88

Copy link
Copy Markdown
Contributor

Fixed make_echo appending the script body instead of overwriting the shebang, and replaced erroneous command-substitution wrappers in assert_ok/assert_not_ok with direct quoted function calls.

Co-authored-by: Андрій Шовкошитний <198119344+enlightened88@users.noreply.github.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
@ljharb

ljharb commented Apr 23, 2026

Copy link
Copy Markdown
Member

@ljharb
ljharb marked this pull request as draft April 23, 2026 17:45
@enlightened88

Copy link
Copy Markdown
Contributor Author

$($FUNCTION $@) runs command substitution instead of calling the function directly - the function's exit code is discarded, breaking the assertion logic entirely.
make_echo used > for both writes, so the second line overwrote the shebang, producing an invalid script file. Changed to >> to append correctly.

@ljharb

ljharb commented Apr 24, 2026

Copy link
Copy Markdown
Member

Can you provide a regression test?

@enlightened88

Copy link
Copy Markdown
Contributor Author

@ljharb ljharb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! I split up the test file and moved it to a place where it'd actually be run.

@ljharb
ljharb marked this pull request as ready for review April 24, 2026 17:27
@ljharb ljharb added the testing Stuff related to testing nvm itself. label Apr 24, 2026
@ljharb ljharb changed the title fix: remove double-substitution in assert_ok and assert_not_ok [tests] remove double-substitution in assert_ok and assert_not_ok Apr 24, 2026
@ljharb
ljharb merged commit 20d52b6 into nvm-sh:master Apr 24, 2026
205 of 207 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing Stuff related to testing nvm itself.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants