From: Junio C Hamano <gitster@pobox•com>
To: Jeff Hostetler <git@jeffhostetler•com>
Cc: git@vger•kernel.org, Jeff Hostetler <jeffhost@microsoft•com>
Subject: Re: [PATCH v5 9/9] status: unit tests for --porcelain=v2
Date: Mon, 08 Aug 2016 10:07:22 -0700 [thread overview]
Message-ID: <xmqq4m6vgpf9.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <1470434434-64283-10-git-send-email-git@jeffhostetler.com> (Jeff Hostetler's message of "Fri, 5 Aug 2016 18:00:34 -0400")
Jeff Hostetler <git@jeffhostetler•com> writes:
> +test_expect_success pre_initial_commit_0 '
> + ...
> + git status --porcelain=v2 --branch --untracked-files=normal >actual &&
> + test_cmp expect actual
> +'
> +
> +
> +test_expect_success pre_initial_commit_1 '
> + git add file_x file_y file_z dir1 &&
> + ...
> + cat >expect <<-EOF &&
> + # branch.oid (initial)
> + # branch.head master
> + 1 A. N... 000000 100644 100644 $_z40 $OID_A dir1/file_a
> + ...
This makes one wonder what would/should be shown on the "A." column
if one of these files are added with "-N" (intent-to-add). I do not
see any test for such entries in this patch, but I think it should.
> + git status --porcelain=v2 --branch --untracked-files=all >actual &&
> + test_cmp expect actual
> +'
> +## Try -z on the above
> +test_expect_success pre_initial_commit_2 '
> + lf_to_nul >expect <<-EOF &&
> + ...
> + git status -z --porcelain=v2 --branch --untracked-files=all >actual &&
> + test_cmp expect actual
> +'
"Try -z on the above" can and should be in the test title to help
those who are watching the test run. Instead of trying to clarify
code with comments, clarify them by letting the code speak for
themselves. I would have named the above three perhaps like this:
test_expect_success 'before first commit, nothing added'
test_expect_success 'before first commit, some files added'
test_expect_success 'before first commit, some files added (-z)'
"pre-initial-commit-$number" is not very interesting; it does not
convey a more interesting aspect of these tests, like (a) _0 is
distinct (nothing added) among the three, (b) _1 and _2 are about
the same state, just expressed differently, and (c) _1 is LF
terminated, _2 is NUL terminated. The same comment applies to the
remainder of the test. For example:
> +##################################################################
> +## Create second commit.
> +##################################################################
> +
> +test_expect_success second_commit_0 '
This "_0" does not tell us anything, but you are testing "When fully
committed, we only see untracked files (if we ask) and branch info
(if we ask).
Having said all that, it is OK to fix their titles after the current
9-patch series lands on 'next'; incremental refinements are easier
on reviewers than having to review too many rerolls.
This is probably a good place to see what happens to these untracked
files and branch info if we do not ask the command to show them.
Otherwise, it tests exactly the same as "initial_commit_0" and is
not all that interesting, no?
> +##################################################################
> +## Ignore a file
> +##################################################################
> +
> +test_expect_success ignore_file_0 '
> + echo x.ign >.gitignore &&
> + echo "ignore me" >x.ign &&
> + H1=$(git rev-parse HEAD) &&
> +
> + ## ignored file SHOULD NOT appear in output when --ignored is not used.
> + ...
> + git status --porcelain=v2 --branch --untracked-files=all >actual &&
> + test_cmp expect actual &&
> + ...
> + git status --porcelain=v2 --branch --ignored --untracked-files=all >actual &&
> + rm x.ign &&
> + rm .gitignore &&
Arrange these files to be cleaned before you create them by having
test_when_finished "rm -f x.ign .gitignore" &&
at the very beginning of this test before they are created.
Otherwise, if any step before these removal fail, later test that
assume they are gone will be affected. You already do so correctly
in the upstream_fields_0 test below.
> +##################################################################
> +## Create some conflicts.
> +##################################################################
> +
> +test_expect_success conflict_AA '
> + git branch AA_A master &&
> + git checkout AA_A &&
> + echo "Branch AA_A" >conflict.txt &&
> + OID_AA_A=$(git hash-object -t blob -- conflict.txt) &&
> + git add conflict.txt &&
> + git commit -m "branch aa_a" &&
> +
> + git branch AA_B master &&
> + git checkout AA_B &&
> + echo "Branch AA_B" >conflict.txt &&
> + OID_AA_B=$(git hash-object -t blob -- conflict.txt) &&
> + git add conflict.txt &&
> + git commit -m "branch aa_b" &&
> +
> + git branch AA_M AA_B &&
> + git checkout AA_M &&
> + test_must_fail git merge AA_A &&
> +
> + HM=$(git rev-parse HEAD) &&
> +
> + cat >expect <<-EOF &&
> + # branch.oid $HM
> + # branch.head AA_M
> + u AA N... 000000 100644 100644 100644 $_z40 $OID_AA_B $OID_AA_A conflict.txt
> + EOF
This is a small point, but doesn't the lowercase 'u' somehow look
ugly, especially because the status letters that immediately follow
it are all uppercase?
> + git status --porcelain=v2 --branch --untracked-files=all >actual &&
> + git reset --hard &&
This "reset" also may be a candidate for test_when_finished clean-up
(I won't repeat the comment but there probably are many others).
> +test_expect_success 'upstream_fields_0' '
> + git checkout master &&
> + test_when_finished rm -rf sub_repo &&
The "when-finished" argument are usually quoted like this, I think:
test_when_finished "rm -rf sub_repo" &&
next prev parent reply other threads:[~2016-08-08 17:07 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-05 22:00 [PATCH v5 0/9] status: V2 porcelain status Jeff Hostetler
2016-08-05 22:00 ` [PATCH v5 1/9] status: rename long-format print routines Jeff Hostetler
2016-08-05 22:00 ` [PATCH v5 2/9] status: cleanup API to wt_status_print Jeff Hostetler
2016-08-05 22:00 ` [PATCH v5 3/9] status: support --porcelain[=<version>] Jeff Hostetler
2016-08-05 22:00 ` [PATCH v5 4/9] status: collect per-file data for --porcelain=v2 Jeff Hostetler
2016-08-05 22:48 ` Junio C Hamano
2016-08-07 8:34 ` Johannes Schindelin
2016-08-07 22:29 ` Eric Wong
2016-08-08 1:57 ` Junio C Hamano
2016-08-05 22:00 ` [PATCH v5 5/9] status: print per-file porcelain v2 status data Jeff Hostetler
2016-08-05 22:51 ` Junio C Hamano
2016-08-05 22:00 ` [PATCH v5 6/9] status: print branch info with --porcelain=v2 --branch Jeff Hostetler
2016-08-05 22:00 ` [PATCH v5 7/9] git-status.txt: describe --porcelain=v2 format Jeff Hostetler
2016-08-05 22:00 ` [PATCH v5 8/9] test-lib-functions.sh: Add lf_to_nul Jeff Hostetler
2016-08-05 22:00 ` [PATCH v5 9/9] status: unit tests for --porcelain=v2 Jeff Hostetler
2016-08-08 17:07 ` Junio C Hamano [this message]
2016-08-10 21:28 ` Jeff Hostetler
2016-08-10 22:41 ` Junio C Hamano
2016-08-10 23:23 ` Jeff Hostetler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq4m6vgpf9.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@jeffhostetler$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=jeffhost@microsoft$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox