threads / patch / 17220

patch, 3 partshttp-push: update tests

Subject: [PATCH 3/3] http-push: update tests

## tl;dr

10 messages between Jan 17, 2009 and Jan 17, 2009. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Ray Chuan· Jan 17, 2009, 02:59 UTC · lore

note: the test needs to chmod the test_repo.git folder so that apache/mod_dav can create .DAV folders in it for locking.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 src/git-1.6.1/t/t5540-http-push.sh |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)
Show changes to src/git-1.6.1/t/t5540-http-push.sh +3 −4
diff --git a/src/git-1.6.1/t/t5540-http-push.sh
b/src/git-1.6.1/t/t5540-http-push.sh
index da95886..730b9cb 100755
--- a/src/git-1.6.1/t/t5540-http-push.sh
+++ b/src/git-1.6.1/t/t5540-http-push.sh
@@ -51,17 +51,16 @@ test_expect_success 'clone remote repository' '
 	git clone $HTTPD_URL/test_repo.git test_repo_clone
 '

-test_expect_failure 'push to remote repository' '
+test_expect_success 'push to remote repository' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	: >path2 &&
 	git add path2 &&
 	test_tick &&
 	git commit -m path2 &&
-	git push &&
-	[ -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master" ]
+	git push origin master
 '

-test_expect_failure 'create and delete remote branch' '
+test_expect_success 'create and delete remote branch' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	git checkout -b dev &&
 	: >path3 &&

-- 
1.6.0.4
Johannes Schindelin· Jan 17, 2009, 05:23 UTC · re: Ray Chuan · lore

Re: [PATCH 3/3] http-push: update tests

Hi,
On Sat, 17 Jan 2009, Ray Chuan wrote:
> note: the test needs to chmod the test_repo.git folder so that
> apache/mod_dav can create .DAV folders in it for locking.
Is this supposed to explain ...
Show 15 quoted lines
> @@ -51,17 +51,16 @@ test_expect_success 'clone remote repository' '
>  	git clone $HTTPD_URL/test_repo.git test_repo_clone
>  '
> 
> -test_expect_failure 'push to remote repository' '
> +test_expect_success 'push to remote repository' '
>  	cd "$ROOT_PATH"/test_repo_clone &&
>  	: >path2 &&
>  	git add path2 &&
>  	test_tick &&
>  	git commit -m path2 &&
> -	git push &&
> -	[ -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master" ]
> +	git push origin master
>  '

... this removal? I do not think this is a good change, as it removes a test that is actually pretty important.

BTW I do not understand at all what you mean by "we need to chmod". Does the Apache instance not run with the current user's permissions?

Ciao, Dscho

Ray Chuan· Jan 17, 2009, 08:40 UTC · re: Johannes Schindelin · lore

Re: [PATCH 3/3] http-push: update tests

Hi,

On Sat, Jan 17, 2009 at 1:23 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 27 quoted lines
> Hi,
>
> On Sat, 17 Jan 2009, Ray Chuan wrote:
>
>> note: the test needs to chmod the test_repo.git folder so that
>> apache/mod_dav can create .DAV folders in it for locking.
>
> Is this supposed to explain ...
>
>> @@ -51,17 +51,16 @@ test_expect_success 'clone remote repository' '
>>       git clone $HTTPD_URL/test_repo.git test_repo_clone
>>  '
>>
>> -test_expect_failure 'push to remote repository' '
>> +test_expect_success 'push to remote repository' '
>>       cd "$ROOT_PATH"/test_repo_clone &&
>>       : >path2 &&
>>       git add path2 &&
>>       test_tick &&
>>       git commit -m path2 &&
>> -     git push &&
>> -     [ -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master" ]
>> +     git push origin master
>>  '
>
> ... this removal?  I do not think this is a good change, as it removes
> a test that is actually pretty important.

i'm sorry for the poor commit message, what i wanted to do was to change the tests to expect success rather than failure. no tests were removed; only their expected outcomes were modified. currently, the pushes fail, so the test 'fails as expected'; now the pushes succeed, so they shouldn't be expecting failed pushes (or anything else).

> BTW I do not understand at all what you mean by "we need to chmod".  Does
> the Apache instance not run with the current user's permissions?

i wrote 'we need to chmod', cos i'm not sure what the permissions are required. sorry, i didn't notice that apache was running with the current user's permissions. but i added the note just in case, for users who don't run apache with the current user, so they modify the test appropriately (ie adding a chmod).

> Ciao,
> Dscho
>
-- 
Cheers,
Ray Chuan
Junio C Hamano· Jan 17, 2009, 18:54 UTC · re: Ray Chuan · lore

Re: [PATCH 3/3] http-push: update tests

"Ray Chuan" <rctay89@gmail.com> writes:
Show 20 quoted lines
>>> -test_expect_failure 'push to remote repository' '
>>> +test_expect_success 'push to remote repository' '
>>>       cd "$ROOT_PATH"/test_repo_clone &&
>>>       : >path2 &&
>>>       git add path2 &&
>>>       test_tick &&
>>>       git commit -m path2 &&
>>> -     git push &&
>>> -     [ -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master" ]
>>> +     git push origin master
>>>  '
>>
>> ... this removal?  I do not think this is a good change, as it removes
>> a test that is actually pretty important.
>
> i'm sorry for the poor commit message, what i wanted to do was to
> change the tests to expect success rather than failure. no tests were
> removed; only their expected outcomes were modified. currently, the
> pushes fail, so the test 'fails as expected'; now the pushes succeed,
> so they shouldn't be expecting failed pushes (or anything else).

The original seems to want the push to succeed, and also it wants the file refs/heads/master to be present after the push (presumably because there should be that ref when the push succeeds). If you fixed "push" that used to fail to succeed, that is great, and s/failure/success/ is a good thing.

But you are removing something else without explanation. Why do you need to remove the part of the test that checks if refs/heads/master is present? Is it looking for a file in a wrong place?

Johannes Schindelin· Jan 17, 2009, 19:37 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] http-push: update tests

Hi,
On Sat, 17 Jan 2009, Junio C Hamano wrote:
Show 9 quoted lines
> The original seems to want the push to succeed, and also it wants the 
> file refs/heads/master to be present after the push (presumably because 
> there should be that ref when the push succeeds).  If you fixed "push" 
> that used to fail to succeed, that is great, and s/failure/success/ is a 
> good thing.
> 
> But you are removing something else without explanation.  Why do you 
> need to remove the part of the test that checks if refs/heads/master is 
> present? Is it looking for a file in a wrong place?

As I mentioned with two other patches, the push does not succeed, and that is the reason for the "failure" in test_expect_failure.

It does not succeed for two reasons:
- due to an off-by-path_len bug, xmalloc() tries to allocate ~4GB of 
  memory, which is a bit much, so http-push die()s with an OOM.
- even with that fix, the push fails because it cannot find any common 
  refs.  It cannot find them because it does not download info/refs as it 
  is supposed to do, but it looks through refs/, missing the fact that the 
  refs are packed (which it cannot handle).

Ciao, Dscho

Ray Chuan· Jan 17, 2009, 19:55 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] http-push: update tests

Hi,
On Sat, Jan 17, 2009 at 6:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> "Ray Chuan" <rctay89@gmail.com> writes:
>
>>>> -test_expect_failure 'push to remote repository' '
>>>> +test_expect_success 'push to remote repository' '
>>>>       cd "$ROOT_PATH"/test_repo_clone &&
>>>>       : >path2 &&
>>>>       git add path2 &&
>>>>       test_tick &&
>>>>       git commit -m path2 &&
>>>> -     git push &&
>>>> -     [ -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master" ]

i modified the push arguments as there was no remote ref/branch specified. With a fixed "git push", that line says:

  No refs in common and none specified; doing nothing.

i'd like to take this chance to inquire, what does the -f, plus square brackets, really mean? i assumed it was to force push to go ahead even if "a remote ref that is not an ancestor of the local ref used to overwrite it" check fails.

-- 
Cheers,
Ray Chuan
Jakub Narebski· Jan 17, 2009, 20:21 UTC · re: Ray Chuan · lore

Re: [PATCH 3/3] http-push: update tests

"Ray Chuan" <rctay89@gmail.com> writes:
Show 12 quoted lines
>>>>> -     git push &&
>>>>> -     [ -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master" ]
> 
> i modified the push arguments as there was no remote ref/branch
> specified. With a fixed "git push", that line says:
> 
>   No refs in common and none specified; doing nothing.
> 
> i'd like to take this chance to inquire, what does the -f, plus square
> brackets, really mean? i assumed it was to force push to go ahead even
> if "a remote ref that is not an ancestor of the local ref used to
> overwrite it" check fails.
Errr...
  git push &&
  [ -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master" ]

means, do "git push", and if it succeeds test (which should really be written as 'test <cond>' and not '[ <cond> ]' I think) if the file does exists and is regular file ('help test' or 'man test').

-- 
Jakub Narebski
Poland
ShadeHawk on #git
Ray Chuan· Jan 17, 2009, 21:00 UTC · re: Jakub Narebski · lore

Re: [PATCH 3/3] http-push: update tests

change tests to expect success.
Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 t/t5540-http-push.sh |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Show changes to t/t5540-http-push.sh +3 −3
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index da95886..4acc087 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -51,17 +51,17 @@ test_expect_success 'clone remote repository' '
 	git clone $HTTPD_URL/test_repo.git test_repo_clone
 '

-test_expect_failure 'push to remote repository' '
+test_expect_success 'push to remote repository' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	: >path2 &&
 	git add path2 &&
 	test_tick &&
 	git commit -m path2 &&
-	git push &&
+	git push origin master &&
 	[ -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master" ]
 '

-test_expect_failure 'create and delete remote branch' '
+test_expect_success 'create and delete remote branch' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	git checkout -b dev &&
 	: >path3 &&

-- 
1.5.6.3
Jakub Narebski· Jan 17, 2009, 21:15 UTC · re: Ray Chuan · lore

Re: [PATCH 3/3] http-push: update tests

Ray Chuan wrote:
> Subject: [PATCH 3/3] http-push: update tests
>
> change tests to expect success.
Minor nit: I would use here
Subject: [PATCH 3/3] http-push: change tests to expect success
-- 
Jakub Narebski
Poland
Johannes Schindelin· Jan 17, 2009, 21:24 UTC · re: Ray Chuan · lore

Re: [PATCH 3/3] http-push: update tests

Hi,
On Sat, 17 Jan 2009, Ray Chuan wrote:
Show 11 quoted lines
> -test_expect_failure 'push to remote repository' '
> +test_expect_success 'push to remote repository' '
>  	cd "$ROOT_PATH"/test_repo_clone &&
>  	: >path2 &&
>  	git add path2 &&
>  	test_tick &&
>  	git commit -m path2 &&
> -	git push &&
> +	git push origin master &&
>  	[ -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master" ]
>  '

But it _should_ succeed without <remote> <branch>. It should find that there is a master locally and one remotely, and push that.

Ciao, Dscho

← back to recent threads