[PATCH] Add more checkout tests

[PATCH] Add more checkout tests

[Daniel Barkalow]

If you have local changes that don't conflict with the
branch-switching changes, these should be kept, not cause errors even
without -m, and be reported afterwards in name-status format.

With -m, the changes carried across should be listed as well. And, for 
now, include the merge-recursive output from this process.

Also test the detatched head message in at least one case.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
 t/t7201-co.sh |   46 +++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 55558ab..3cf0cf1 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -20,6 +20,8 @@ Test switching across them.
 
 . ./test-lib.sh
 
+test_tick
+
 fill () {
 	for i
 	do
@@ -30,9 +32,10 @@ fill () {
 
 test_expect_success setup '
 
+	fill x y z > same &&
 	fill 1 2 3 4 5 6 7 8 >one &&
 	fill a b c d e >two &&
-	git add one two &&
+	git add same one two &&
 	git commit -m "Initial A one, A two" &&
 
 	git checkout -b renamer &&
@@ -74,16 +77,44 @@ test_expect_success "checkout with dirty tree without -m" '
 
 '
 
+test_expect_success "checkout with unrelated dirty tree without -m" '
+
+	git checkout -f master &&
+	fill 0 1 2 3 4 5 6 7 8 >same &&
+	cp same kept
+	git checkout side >messages && 
+	git diff same kept
+	(cat > messages.expect <<EOF
+M	same
+EOF
+) &&
+	touch messages.expect &&
+	git diff messages.expect messages
+'
+
 test_expect_success "checkout -m with dirty tree" '
 
 	git checkout -f master &&
 	git clean -f &&
 
 	fill 0 1 2 3 4 5 6 7 8 >one &&
-	git checkout -m side &&
+	git checkout -m side > messages &&
 
 	test "$(git symbolic-ref HEAD)" = "refs/heads/side" &&
 
+	(cat >expect.messages <<EOF
+Merging side with local
+Merging:
+ab76817 Side M one, D two, A three
+virtual local
+found 1 common ancestor(s):
+7329388 Initial A one, A two
+Auto-merged one
+M	one
+EOF
+) &&
+	git diff expect.messages messages &&
+
 	fill "M	one" "A	three" "D	two" >expect.master &&
 	git diff --name-status master >current.master &&
 	diff expect.master current.master &&
@@ -145,7 +176,16 @@ test_expect_success 'checkout -m with merge conflict' '
 test_expect_success 'checkout to detach HEAD' '
 
 	git checkout -f renamer && git clean -f &&
-	git checkout renamer^ &&
+	git checkout renamer^ 2>messages &&
+	(cat >messages.expect <<EOF
+Note: moving to "renamer^" which isn'"'"'t a local branch
+If you want to create a new branch from this checkout, you may do so
+(now or later) by using -b with the checkout command again. Example:
+  git checkout -b <new_branch_name>
+HEAD is now at 7329388... Initial A one, A two
+EOF
+) &&
+	git diff messages.expect messages &&
 	H=$(git rev-parse --verify HEAD) &&
 	M=$(git show-ref -s --verify refs/heads/master) &&
 	test "z$H" = "z$M" &&
-- 
1.5.3.6.886.gb204

Re: [PATCH] Add more checkout tests

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> +test_expect_success "checkout with unrelated dirty tree without -m" '
> +
> +	git checkout -f master &&
> +	fill 0 1 2 3 4 5 6 7 8 >same &&
> +	cp same kept
> +	git checkout side >messages && 
> +	git diff same kept
> +	(cat > messages.expect <<EOF
> +M	same
> +EOF
> +) &&
> +	touch messages.expect &&
> +	git diff messages.expect messages
> +'

What is this "touch" about?

I do not recall the details, but we had problem reports that some shells
do not handle here-documents (i.e. cmd <<EOF) inside test_expect_success
well, and generally tried to keep them outside.  I however see some of
the newer tests do have here-doc inside expect-success, and do not
recall hearing breakage reports on them.  Maybe it was a false alarm and
we were being overly cautious, or maybe not enough people (especially on
more exotic systems) are running tests these days.  Let's keep the
here-doc as-is in your tests and see what happens.

>  test_expect_success "checkout -m with dirty tree" '
>  
>  	git checkout -f master &&
>  	git clean -f &&
>  
>  	fill 0 1 2 3 4 5 6 7 8 >one &&
> -	git checkout -m side &&
> +	git checkout -m side > messages &&
>  
>  	test "$(git symbolic-ref HEAD)" = "refs/heads/side" &&
>  
> +	(cat >expect.messages <<EOF
> +Merging side with local
> +Merging:
> +ab76817 Side M one, D two, A three
> +virtual local
> +found 1 common ancestor(s):
> +7329388 Initial A one, A two
> +Auto-merged one
> +M	one
> +EOF
> +) &&
> +	git diff expect.messages messages &&

I do not like the idea of testing the exact wording of messages this
way.

I do not think we care about the exact wording of these messages, and I
think our tests should check what we do care about without casting the
UI in stone.  Otherwise, it will make it harder to clean-up the user
experience later.  Perhaps it would be sufficient to make sure that (1)
this checkout succeeds with exit 0 status, and that (2) the contents of
the merged 'one' is a reasonable merge result, i.e. "git diff HEAD one"
gets the same patch-id as "git diff HEAD one" taken before switching the
branches.

> @@ -145,7 +176,16 @@ test_expect_success 'checkout -m with merge conflict' '
>  test_expect_success 'checkout to detach HEAD' '
>  
>  	git checkout -f renamer && git clean -f &&
> -	git checkout renamer^ &&
> +	git checkout renamer^ 2>messages &&
> +	(cat >messages.expect <<EOF
> +Note: moving to "renamer^" which isn'"'"'t a local branch
> +If you want to create a new branch from this checkout, you may do so
> +(now or later) by using -b with the checkout command again. Example:
> +  git checkout -b <new_branch_name>
> +HEAD is now at 7329388... Initial A one, A two
> +EOF
> +) &&
> +	git diff messages.expect messages &&

Same here.  If we want to make sure the head is detached at the intended
commit, make sure "rev-parse HEAD" gives the expected result, and make
sure "symbolic-ref HEAD" says it is not symbolic.

>  	H=$(git rev-parse --verify HEAD) &&
>  	M=$(git show-ref -s --verify refs/heads/master) &&
>  	test "z$H" = "z$M" &&
> -- 
> 1.5.3.6.886.gb204

Re: [PATCH] Add more checkout tests

[Daniel Barkalow]

On Sun, 9 Dec 2007, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > +test_expect_success "checkout with unrelated dirty tree without -m" '
> > +
> > +	git checkout -f master &&
> > +	fill 0 1 2 3 4 5 6 7 8 >same &&
> > +	cp same kept
> > +	git checkout side >messages && 
> > +	git diff same kept
> > +	(cat > messages.expect <<EOF
> > +M	same
> > +EOF
> > +) &&
> > +	touch messages.expect &&
> > +	git diff messages.expect messages
> > +'
> 
> What is this "touch" about?

Left over from before I'd added the here document, so there'd be a file to 
diff against, and it would be wrong, and I could find the actual contents. 
I just forgot to take it out when I was creating the real thing.

> I do not recall the details, but we had problem reports that some shells
> do not handle here-documents (i.e. cmd <<EOF) inside test_expect_success
> well, and generally tried to keep them outside.  I however see some of
> the newer tests do have here-doc inside expect-success, and do not
> recall hearing breakage reports on them.  Maybe it was a false alarm and
> we were being overly cautious, or maybe not enough people (especially on
> more exotic systems) are running tests these days.  Let's keep the
> here-doc as-is in your tests and see what happens.

Sure. Possibly we should instead just be testing for the presence of the 
correct line, and absence of incorrect lines, in any case?

> >  test_expect_success "checkout -m with dirty tree" '
> >  
> >  	git checkout -f master &&
> >  	git clean -f &&
> >  
> >  	fill 0 1 2 3 4 5 6 7 8 >one &&
> > -	git checkout -m side &&
> > +	git checkout -m side > messages &&
> >  
> >  	test "$(git symbolic-ref HEAD)" = "refs/heads/side" &&
> >  
> > +	(cat >expect.messages <<EOF
> > +Merging side with local
> > +Merging:
> > +ab76817 Side M one, D two, A three
> > +virtual local
> > +found 1 common ancestor(s):
> > +7329388 Initial A one, A two
> > +Auto-merged one
> > +M	one
> > +EOF
> > +) &&
> > +	git diff expect.messages messages &&
> 
> I do not like the idea of testing the exact wording of messages this
> way.
> 
> I do not think we care about the exact wording of these messages, and I
> think our tests should check what we do care about without casting the
> UI in stone.  Otherwise, it will make it harder to clean-up the user
> experience later.  Perhaps it would be sufficient to make sure that (1)
> this checkout succeeds with exit 0 status, and that (2) the contents of
> the merged 'one' is a reasonable merge result, i.e. "git diff HEAD one"
> gets the same patch-id as "git diff HEAD one" taken before switching the
> branches.

What I'm actually testing for is the "M<tab>one" line, and that the 
previous line isn't name-status stuff; that is, that the name-status stuff 
is right.

Of course, a patch to clean up the user experience could have a hunk that 
makes the test expect that the UI is cleaned up. It's not like we can't 
change our tests to accompany improvements in behavior, and I'd argue that 
those hunks give a useful example of the improvement.

> > @@ -145,7 +176,16 @@ test_expect_success 'checkout -m with merge conflict' '
> >  test_expect_success 'checkout to detach HEAD' '
> >  
> >  	git checkout -f renamer && git clean -f &&
> > -	git checkout renamer^ &&
> > +	git checkout renamer^ 2>messages &&
> > +	(cat >messages.expect <<EOF
> > +Note: moving to "renamer^" which isn'"'"'t a local branch
> > +If you want to create a new branch from this checkout, you may do so
> > +(now or later) by using -b with the checkout command again. Example:
> > +  git checkout -b <new_branch_name>
> > +HEAD is now at 7329388... Initial A one, A two
> > +EOF
> > +) &&
> > +	git diff messages.expect messages &&
> 
> Same here.  If we want to make sure the head is detached at the intended
> commit, make sure "rev-parse HEAD" gives the expected result, and make
> sure "symbolic-ref HEAD" says it is not symbolic.

I think we're already testing for that. I really want the "HEAD is now..." 
line, which ought to give the right info.

The point of adding these tests (and parts of tests) is that I'd forgotten 
to maintain some of the important information while writing 
builtin-checkout, and there wasn't a test that it was provided. While some 
of the output is arbitrary informative text, there's a certain amount of 
generated information there that shouldn't get lost or be incorrect.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Add more checkout tests

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> Of course, a patch to clean up the user experience could have a hunk that 
> makes the test expect that the UI is cleaned up. It's not like we can't 
> change our tests to accompany improvements in behavior, and I'd argue that 
> those hunks give a useful example of the improvement.

Ok.

> The point of adding these tests (and parts of tests) is that I'd forgotten 
> to maintain some of the important information while writing 
> builtin-checkout, and there wasn't a test that it was provided. While some 
> of the output is arbitrary informative text, there's a certain amount of 
> generated information there that shouldn't get lost or be incorrect.

Fair enough.