Re: [PATCH 2/5] t0061: Add tests

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Frans Klaver wrote:

> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -13,6 +13,24 @@ cat hello-script
>  EOF
>  >empty
>  
> +cat >someinterpreter <<-EOF
> +#!$SHELL_PATH
> +cat hello-script
> +EOF
> +>empty
> +
> +cat >incorrect-interpreter-script <<-EOF
> +#!someinterpreter
> +cat hello-script
> +EOF
> +>empty

Thanks for writing tests.  Some hints on mechanics below, and one on
strategy (see (*) below).

What is the point of repreatedly writing an empty file named "empty"?

I think this would be easier to read and maintain if scripts not
shared between multiple tests were written in the body of the relevant
tests.  For example, that way it is easier to remember to remove a
helper script if the relevant test assertion changes to no longer need
it.

[...]
> @@ -26,7 +44,7 @@ test_expect_success 'run_command can run a command' '
>  	test_cmp empty err
>  '
>  
>  test_expect_success POSIXPERM 'run_command reports EACCES' '
>  	cat hello-script >hello.sh &&
>  	chmod -x hello.sh &&
>  	test_must_fail test-run-command run-command ./hello.sh 2>err &&
[...]
> +test_expect_success POSIXPERM 'run_command reports EACCES, search path perms' '
> +	mkdir -p inaccessible &&
> +	PATH=$(pwd)/inaccessible:$PATH &&
> +	export PATH &&
> +
> +	cat hello-script >inaccessible/hello.sh &&
> +	chmod 400 inaccessible &&
> +	test_must_fail test-run-command run-command hello.sh 2>err &&
> +	chmod 755 inaccessible &&
> +
> +	grep "fatal: cannot exec.*hello.sh" err
> +'

(*) These tests would be easier to understand if squashed with the
relevant later patch in the series that changes the error message.

Maybe they could be less repetitive that way, too.

	test_expect_success POSIXPERM 'diagnose command in inaccessible part of $PATH' '
		mkdir -p subdir &&
		cat hello-script >subdir/hello.sh &&
		chmod +x subdir/hello.sh &&
		chmod -x subdir &&
		(
			PATH=$(pwd)/inaccessible:$PATH &&
			test_must_fail test-run-command run-command hello.sh 2>err
		) &&
		test_i18ngrep ...
	'

[...]
> +test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '
> +	cat incorrect-interpreter-script >hello.sh &&
> +	chmod +x hello.sh &&
> +	chmod -x someinterpreter &&
> +	test_must_fail test-run-command run-command ./hello.sh 2>err &&
> +
> +	grep "fatal: cannot exec.*hello.sh" err
> +'

Is this the common case?  Why would my interpreter be in the designated
spot but not marked executable?  Is there some other motivating
example?  (I'm genuinely curious; it's ok if the answer is "no".)

[...]
> +
> +test_expect_failure POSIXPERM 'run_command reports ENOENT, interpreter' '
> +	cat non-existing-interpreter >hello.sh &&
> +	chmod +x hello.sh &&
> +	test_must_fail test-run-command start-command-ENOENT ./hello.sh 2>err &&
> +
> +	grep "error: cannot exec.*hello.sh" err
> +'

Maybe:

	test_expect_success POSIXPERM 'diagnose missing interpreter' '
		echo "#!/nonexistent/interpreter" >hello.sh &&
		chmod +x hello.sh &&
		test_must_fail test-run-command run-command hello.sh 2>err &&
		test_i18ngrep ...
	'

Hope that helps,
Jonathan