Re: [PATCH v4 8/8] status: tests for --porcelain=v2

[Jeff Hostetler <git@jeffhostetler.com>]

On 08/05/2016 02:12 PM, Junio C Hamano wrote:
> Jeff Hostetler <git@jeffhostetler.com> writes:
>
>> +##################################################################
>> +## Confirm output prior to initial commit.
>> +##################################################################
>> +
>> +test_expect_success pre_initial_commit_0 '
>
> Bikeshedding, but our codebase seems to prefer "expect" vs "actual".
>
>      $ git grep -e 'test_cmp expect ' t/ | wc -l
>      1882
>      $ git grep -e 'test_cmp expected ' t/ | wc -l
>      888
>
>> +	cat >expected <<-EOF &&
>> +	# branch.oid (initial)
>> +	# branch.head master
>> +	? actual
>> +	? dir1/
>> +	? expected
>> +	? file_x
>> +	? file_y
>> +	? file_z
>> +	EOF
>
> Perhaps throw these two entries to .gitignore to allow new tests in
> the future could also use expect.1 vs actual.1 and somesuch?
>
> 	cat >.gitignore <<-\EOF &&
> 	expect*
>          actual*
>          EOF
>
>> +test_expect_success pre_initial_commit_1 '
>> +	git add file_x file_y file_z dir1 &&
>> +	SHA_A=`git hash-object -t blob -- dir1/file_a` &&
>> +	SHA_B=`git hash-object -t blob -- dir1/file_b` &&
>> +	SHA_X=`git hash-object -t blob -- file_x` &&
>> +	SHA_Y=`git hash-object -t blob -- file_y` &&
>> +	SHA_Z=`git hash-object -t blob -- file_z` &&
>
> Please use $(commannd) instead of `command`.  Also "SHA" is probably
> a bad prefix; either use "SHA_1" to be technically correct, or
> better yet use "OID", as we are moving towards abstracting the exact
> hash function name away.
>
>> +	SHA_ZERO=0000000000000000000000000000000000000000 &&
>
> I think we made $_z40 available to you from t/test-lib.sh.
>
>> +## Try -z on the above
>> +test_expect_success pre_initial_commit_2 '
>> +	cat >expected.lf <<-EOF &&
>> +	# branch.oid (initial)
>> +	# branch.head master
>> +	1 A. N... 000000 100644 100644 $SHA_ZERO $SHA_A dir1/file_a
>> +	1 A. N... 000000 100644 100644 $SHA_ZERO $SHA_B dir1/file_b
>> +	1 A. N... 000000 100644 100644 $SHA_ZERO $SHA_X file_x
>> +	1 A. N... 000000 100644 100644 $SHA_ZERO $SHA_Y file_y
>> +	1 A. N... 000000 100644 100644 $SHA_ZERO $SHA_Z file_z
>> +	? actual
>> +	? expected
>> +	EOF
>> +	perl -pe y/\\012/\\000/ <expected.lf >expected &&
>> +	rm expected.lf &&
>
> As you immediately remove expected.lf, the first "cat" process is
> rather pointless.  You can redirect here text <<-EOF directly into
> perl instead.  Also it would probably help to add a new helper
> "lf_to_nul" in t/test-lib-functions.sh around the place where
> nul_to_q, ..., tz_to_tab_space helpers are defined, which would
> allow us to say
>
> 	lf_to_nul >expect <<-EOF &&
> 	...
>          EOF
>
>> +test_expect_success initial_commit_3 '
>> +	git mv file_y renamed_y &&
>> +	H0=`git rev-parse HEAD` &&
>> +
>> +	cat >expected.q <<-EOF &&
>> +	# branch.oid $H0
>> +	# branch.head master
>> +	1 M. N... 100644 100644 100644 $SHA_X $SHA_X1 file_x
>> +	1 D. N... 100644 000000 000000 $SHA_Z $SHA_ZERO file_z
>> +	2 R. N... 100644 100644 100644 $SHA_Y $SHA_Y R100 renamed_yQfile_y
>> +	? actual
>> +	? expected
>> +	EOF
>> +	q_to_tab <expected.q >expected &&
>> +	rm expected.q &&
>
> The same comment applies (redirect directly into q_to_tab).
>
>> +##################################################################
>> +## Ignore a file
>> +##################################################################
>> +
>> +test_expect_success ignore_file_0 '
>> +	echo x.ign >.gitignore &&
>> +	echo "ignore me" >x.ign &&
>> +	H1=`git rev-parse HEAD` &&
>> +
>> +	cat >expected <<-EOF &&
>> +	# branch.oid $H1
>> +	# branch.head master
>> +	? .gitignore
>> +	? actual
>> +	? expected
>> +	! x.ign
>> +	EOF
>> +
>> +	git status --porcelain=v2 --branch --ignored --untracked-files=all >actual &&
>> +	rm x.ign &&
>> +	rm .gitignore &&
>> +	test_cmp expected actual
>> +'
>
> You do not seem to be checking a feature is not triggered when not
> asked throughout this test, e.g. making sure the output does not
> have the "# branch.*" lines when --branch is not given, "! x.ign"
> is not shown when --ignored is not given, etc.
>
>> +##################################################################
>> +## Test upstream fields in branch header
>> +##################################################################
>> +
>> +test_expect_success 'upstream_fields_0' '
>> +	git checkout master &&
>> +	git clone . sub_repo &&
>> +	(
>> +		## Confirm local master tracks remote master.
>> +		cd sub_repo &&
>> +		HUF=`git rev-parse HEAD` &&
>> + ...
>> +		git status --porcelain=v2 --branch --ignored --untracked-files=all >actual &&
>> +		test_cmp expected actual
>> +	) &&
>> +	rm -rf sub_repo
>
> It probably is a good idea to use test_when_finished immediately
> before "git clone . sub_repo" to arrange this to happen even when
> any test in the subshell fails.
>


Lots of good points here (and on the earlier commits in this
series).  I'll address and send up a new version shortly.

Thanks!
Jeff