threads / patch / 11217

patchAdd more checkout tests

Subject: [PATCH] Add more checkout tests

## tl;dr

4 messages between Dec 10, 2007 and Dec 10, 2007. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Daniel Barkalow· Dec 10, 2007, 03:05 UTC · lore

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(-)
Show changes to t/t7201-co.sh +43 −3
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
Junio C Hamano· Dec 10, 2007, 03:42 UTC · re: Daniel Barkalow · lore

Re: [PATCH] Add more checkout tests

Daniel Barkalow <barkalow@iabervon.org> writes:
Show 14 quoted lines
> +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.

Show 23 quoted lines
>  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.

Show 15 quoted lines
> @@ -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.

Show 5 quoted lines
>  	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
Daniel Barkalow· Dec 10, 2007, 04:03 UTC · re: Junio C Hamano · lore

Re: [PATCH] Add more checkout tests

On Sun, 9 Dec 2007, Junio C Hamano wrote:
Show 18 quoted lines
> 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.

Show 8 quoted lines
> 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?

Show 35 quoted lines
> >  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.

Show 19 quoted lines
> > @@ -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*
Junio C Hamano· Dec 10, 2007, 05:14 UTC · re: Daniel Barkalow · lore

Re: [PATCH] Add more checkout tests

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.
Show 5 quoted lines
> 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.

← back to recent threads