[PATCH] test-lib: avoid full path to store test results

[PATCH] test-lib: avoid full path to store test results

[Felipe Contreras]

No reason to use the full path in case this is used externally.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/test-lib.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 514282c..5a3d665 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -389,7 +389,8 @@ test_done () {
 	then
 		test_results_dir="$TEST_OUTPUT_DIRECTORY/test-results"
 		mkdir -p "$test_results_dir"
-		test_results_path="$test_results_dir/${0%.sh}-$$.counts"
+		base=${0##*/}
+		test_results_path="$test_results_dir/${base%.sh}-$$.counts"
 
 		cat >>"$test_results_path" <<-EOF
 		total $test_count
-- 
1.8.0

Re: [PATCH] test-lib: avoid full path to store test results

[Jeff King]

On Tue, Oct 30, 2012 at 05:12:57AM +0100, Felipe Contreras wrote:

> No reason to use the full path in case this is used externally.

I think it is not just "no reason to", but it is actively wrong to use a
full path, as we do not take care to "mkdir -p" the intervening path
components.

However, this never comes up in practice, because all of the test
scripts assume you are running them from the test directory (i.e.,
they will fail otherwise because they will not find ./test-lib.sh).

Is this in support of putting remote-hg tests in contrib/? I had
expected you to just put

  export TEST_DIRECTORY="$(pwd)/../../../t"
  . "$TEST_DIRECTORY/test-lib.sh"

into the test script in contrib/remote-hg/t. I guess you are doing
something like:

  cd ../../../t && ../contrib/remote-hg/t/twhatever...

but the former seems much simpler to invoke (and if the goal is to get
your test-results in the right place, setting TEST_OUTPUT_DIRECTORY is
the best way to do that).

If this is part of the remote-hg series, I'd prefer to just see it as
part of the re-roll. It's much easier to evaluate it in context.

Or are you really just doing:

  cd git/t
  $PWD/t0000-basic.sh

I guess there is nothing wrong with that, though there is no reason not
to use "./" instead of $PWD.

-Peff

Re: [PATCH] test-lib: avoid full path to store test results

[Felipe Contreras]

On Tue, Oct 30, 2012 at 5:28 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Oct 30, 2012 at 05:12:57AM +0100, Felipe Contreras wrote:
>
>> No reason to use the full path in case this is used externally.
>
> I think it is not just "no reason to", but it is actively wrong to use a
> full path, as we do not take care to "mkdir -p" the intervening path
> components.
>
> However, this never comes up in practice, because all of the test
> scripts assume you are running them from the test directory (i.e.,
> they will fail otherwise because they will not find ./test-lib.sh).
>
> Is this in support of putting remote-hg tests in contrib/? I had
> expected you to just put
>
>   export TEST_DIRECTORY="$(pwd)/../../../t"
>   . "$TEST_DIRECTORY/test-lib.sh"

If there was a single script and we didn't want reports, sure, but
this is not too bad:

TESTS := $(wildcard test*.sh)

export T := $(addprefix $(CURDIR)/,$(TESTS))
export MAKE := $(MAKE) -e
export PATH := $(CURDIR):$(PATH)

test:
	$(MAKE) -C ../../t $@

$(TESTS):
	$(MAKE) -C ../../t $(CURDIR)/$@

.PHONY: $(TESTS)

I just sent the new remote-hg patch series with that.

Cheers.

-- 
Felipe Contreras

Re: [PATCH] test-lib: avoid full path to store test results

[Jonathan Nieder]

Hi,

Felipe Contreras wrote:

> No reason to use the full path in case this is used externally.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

"No reason not to" is not a reason to do anything.  What symptoms does
this prevent?  Could you describe the benefit of this patch in a
paragraph starting "Now you can ..."?

Thanks,
Jonathan

Re: [PATCH] test-lib: avoid full path to store test results

[Elia Pinto]

The shell word splitting done in base is a bashism, iow not portable.

Best

2012/10/30, Felipe Contreras <felipe.contreras@gmail.com>:
> No reason to use the full path in case this is used externally.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/test-lib.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 514282c..5a3d665 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -389,7 +389,8 @@ test_done () {
>  	then
>  		test_results_dir="$TEST_OUTPUT_DIRECTORY/test-results"
>  		mkdir -p "$test_results_dir"
> -		test_results_path="$test_results_dir/${0%.sh}-$$.counts"
> +		base=${0##*/}
> +		test_results_path="$test_results_dir/${base%.sh}-$$.counts"
>
>  		cat >>"$test_results_path" <<-EOF
>  		total $test_count
> --
> 1.8.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Inviato dal mio dispositivo mobile

Re: [PATCH] test-lib: avoid full path to store test results

[Jonathan Nieder]

Elia Pinto wrote:

> The shell word splitting done in base is a bashism, iow not portable.

No, ${varname##glob} is in POSIX and we already use it here and there.
See Documentation/CodingGuidelines:

   - We use ${parameter#word} and its [#%] siblings, and their
     doubled "longest matching" form.

Thanks for looking the patch over.
Jonathan

Re: [PATCH] test-lib: avoid full path to store test results

[Felipe Contreras]

On Tue, Oct 30, 2012 at 5:46 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> No reason to use the full path in case this is used externally.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>
> "No reason not to" is not a reason to do anything.  What symptoms does
> this prevent?  Could you describe the benefit of this patch in a
> paragraph starting "Now you can ..."?

./test-lib.sh: line 394:
/home/felipec/dev/git/t/test-results//home/felipec/dev/git/contrib/remote-hg/test-21865.counts:
No such file or directory

-- 
Felipe Contreras

Re: [PATCH] test-lib: avoid full path to store test results

[Elia Pinto]

Thanks. I know that posix support these usages, but exists some
traditional shell that not support it. These are described in the
autoconf manual, last time i have checked. As the construct ; export
var = x should be portable, but it is not. If this is important these
days i don't know.


Best

2012/10/30, Jonathan Nieder <jrnieder@gmail.com>:
> Elia Pinto wrote:
>
>> The shell word splitting done in base is a bashism, iow not portable.
>
> No, ${varname##glob} is in POSIX and we already use it here and there.
> See Documentation/CodingGuidelines:
>
>    - We use ${parameter#word} and its [#%] siblings, and their
>      doubled "longest matching" form.
>
> Thanks for looking the patch over.
> Jonathan
>

-- 
Inviato dal mio dispositivo mobile

Re: [PATCH] test-lib: avoid full path to store test results

[Jonathan Nieder]

Felipe Contreras wrote:
> On Tue, Oct 30, 2012 at 5:46 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> > Felipe Contreras wrote:

>>> No reason to use the full path in case this is used externally.
>>>
>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>
>> "No reason not to" is not a reason to do anything.  What symptoms does
>> this prevent?  Could you describe the benefit of this patch in a
>> paragraph starting "Now you can ..."?
>
> ./test-lib.sh: line 394:
> /home/felipec/dev/git/t/test-results//home/felipec/dev/git/contrib/remote-hg/test-21865.counts:
> No such file or directory

Ok, so a description for this patch is

	test: use test's basename to name results file

	Running a test using its full path produces an error:

		$ ~/dev/git/contrib/remote-hg/test-21865.sh
	[...]
		./test-lib.sh: line 394: /home/felipec/dev/t/\
		test-results//home/felipec/dev/git/contrib/remote-hg/\
		test-21865.counts: No such file or directory

	In --tee and --valgrind modes we already use the basename
	to name the .out and .exit files; this patch teaches the test-lib
	to name the .counts file the same way.

That is still not enough to tell if it is a good change, though.
Should the test results for contrib/remote-hg/test-* be included with
the results for t/t*.sh when I run "make aggregate-results"?

Before 60d02ccc, git-svn had its own testsuite under contrib/, with
glue in contrib/git-svn/t/lib-git-svn.sh to use test-lib --- maybe
that code could provide some inspiration for questions like these.

Hope that helps,
Jonathan

Re: [PATCH] test-lib: avoid full path to store test results

[Felipe Contreras]

On Wed, Oct 31, 2012 at 2:27 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>> On Tue, Oct 30, 2012 at 5:46 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> > Felipe Contreras wrote:
>
>>>> No reason to use the full path in case this is used externally.
>>>>
>>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>>
>>> "No reason not to" is not a reason to do anything.  What symptoms does
>>> this prevent?  Could you describe the benefit of this patch in a
>>> paragraph starting "Now you can ..."?
>>
>> ./test-lib.sh: line 394:
>> /home/felipec/dev/git/t/test-results//home/felipec/dev/git/contrib/remote-hg/test-21865.counts:
>> No such file or directory
>
> Ok, so a description for this patch is
>
>         test: use test's basename to name results file

Is this solving an actual problem or is it just something nice to do?
Like in all good novels, one has to read more find out...

>         Running a test using its full path produces an error:

I'm not sure what that even means. Do you mean this produces an error?

% make -C t $PWD/t9902-completion.sh

Well, sure it does, but this patch doesn't fix that.

If you want a precise explanation of what kind of usages are enabled
by this patch, that would require some work, and no I haven't done it,
and no, I'm not sure.

>                 $ ~/dev/git/contrib/remote-hg/test-21865.sh
>         [...]
>                 ./test-lib.sh: line 394: /home/felipec/dev/t/\
>                 test-results//home/felipec/dev/git/contrib/remote-hg/\
>                 test-21865.counts: No such file or directory

Except that I didn't do this. So the fact that this happens is an
assumption, and I'm not willing to make that.

Most likely if somebody does that they are doing something wrong; they
didn't define the TESTDIR variable (or something like that).

It's all fun and games to write explanations for things, but it's not
that easy when you want those explanations to be actually true, and
corrent--you have to spend time to make sure of that.

>         In --tee and --valgrind modes we already use the basename
>         to name the .out and .exit files; this patch teaches the test-lib
>         to name the .counts file the same way.

I don't see the point of listing each and every place where this
already happens. As a matter of fact, the base-name is used in other
places as well, and just saying "This is already done in other
places", is more than enough. But who says they are not the ones doing
it wrong? Maybe this part of the code is right, and it's the others
that need fixing. I don't see how saying "Others are doing it" makes
the patch better or worse in any way. There might also be different
reasons for why they do it that doesn't apply here.

> That is still not enough to tell if it is a good change, though.
> Should the test results for contrib/remote-hg/test-* be included with
> the results for t/t*.sh when I run "make aggregate-results"?
>
> Before 60d02ccc, git-svn had its own testsuite under contrib/, with
> glue in contrib/git-svn/t/lib-git-svn.sh to use test-lib --- maybe
> that code could provide some inspiration for questions like these.

Or maybe they are the ones that should look for inspiration in
contrib/remote-hg.

The patch is obviously correct; it's generally good not to name files
with slashes in them, and $0 is not guaranteed not to have slashes.
Even if you run all the tests inside the 't' directory, this script is
not only used by git, and others might want sub-directories, and not
thousands of tests on the same directory like git.

Either way, if obvious fixes that are one-liners require an essay for
you, I give up.

Cheers.

-- 
Felipe Contreras

Re: [PATCH] test-lib: avoid full path to store test results

[Jonathan Nieder]

Felipe Contreras wrote:

> It's all fun and games to write explanations for things, but it's not
> that easy when you want those explanations to be actually true, and
> corrent--you have to spend time to make sure of that.

That's why it's useful for the patch submitter to write them, asking
for help when necessary.

As a bonus, it helps reviewers understand the effect of the patch.
Bugs averted!

[...]
> Either way, if obvious fixes that are one-liners require an essay for
> you, I give up.

I guess it is fair to call a reasonable subject line plus a couple of
sentences an essay.  Yes, obvious fixes especially require that, since
the bug caused by an obvious fix is one of the worst kinds.

Re: [PATCH] test-lib: avoid full path to store test results

[Felipe Contreras]

On Wed, Oct 31, 2012 at 3:13 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> It's all fun and games to write explanations for things, but it's not
>> that easy when you want those explanations to be actually true, and
>> corrent--you have to spend time to make sure of that.
>
> That's why it's useful for the patch submitter to write them, asking
> for help when necessary.
>
> As a bonus, it helps reviewers understand the effect of the patch.
> Bugs averted!

Yeah, that would be nice. Too bad I don't have that information, and
have _zero_ motivation to go and get it for you.

> [...]
>> Either way, if obvious fixes that are one-liners require an essay for
>> you, I give up.
>
> I guess it is fair to call a reasonable subject line plus a couple of
> sentences an essay.  Yes, obvious fixes especially require that, since
> the bug caused by an obvious fix is one of the worst kinds.

Yes, I've written essays for one-line fixes, in the Linux kernel,
where details matter, and things are not so obvious.

This is not the case here. This is as obvious and simple as things
get. If there was a problem with it, somebody would have found it by
now.

Let not the perfect be the enemy of the good. Or do. Up to you.

-- 
Felipe Contreras

Re: [PATCH] test-lib: avoid full path to store test results

[Stefano Lattarini]

On 10/30/2012 11:17 PM, Elia Pinto wrote:
> Thanks. I know that posix support these usages, but exists some
> traditional shell that not support it.
>
True, but those shells are not POSIX shells -- the major example that
comes to mind is the accursed Solaris /bin/sh.

Since Git assumes a POSIX shell in its scripts and testsuite, use of
any POSIX feature should be fine -- until someone can show a real-world
POSIX shell that (likely due to a bug) fails to grasp such feature, in
which case a "pragmatic" workaround is needed.

Oh, and BTW, there are talks (and mostly consensus) among the Autotools
developers to start requiring a POSIX shell in the configure scripts
and Makefile recipes in the near future:

  <http://lists.gnu.org/archive/html/bug-autoconf/2012-06/msg00009.html>

And also, related:

  <http://lists.gnu.org/archive/html/automake/2012-08/msg00046.html>
  <http://lists.gnu.org/archive/html/coreutils/2012-10/msg00127.html>

>These are described in the
> autoconf manual, last time i have checked. As the construct ; export
> var = x should be portable, but it is not.
>
I don't think POSIX requires that to be portable.

> If this is important these days i don't know.
>
I hope the above helps to clarify the matter a little.

Regards,
  Stefano

Re: [PATCH] test-lib: avoid full path to store test results

[Johannes Sixt]

Am 31.10.2012 03:28, schrieb Felipe Contreras:
> On Wed, Oct 31, 2012 at 3:13 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Felipe Contreras wrote:
>>
>>> It's all fun and games to write explanations for things, but it's not
>>> that easy when you want those explanations to be actually true, and
>>> corrent--you have to spend time to make sure of that.
>>
>> That's why it's useful for the patch submitter to write them, asking
>> for help when necessary.
>>
>> As a bonus, it helps reviewers understand the effect of the patch.
>> Bugs averted!
> 
> Yeah, that would be nice. Too bad I don't have that information, and
> have _zero_ motivation to go and get it for you.

Just to clarify: That information is not just for Jonathan, but for
everyone on this list and those who dig the history a year down the
road. Contributors who have _zero_ motiviation to find out that
information are not welcome here because they cause friction and take
away time from many others for _zero_ gain.

-- Hannes

Re: [PATCH] test-lib: avoid full path to store test results

[Felipe Contreras]

On Wed, Oct 31, 2012 at 7:02 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 31.10.2012 03:28, schrieb Felipe Contreras:

>> Yeah, that would be nice. Too bad I don't have that information, and
>> have _zero_ motivation to go and get it for you.
>
> Just to clarify: That information is not just for Jonathan, but for
> everyone on this list and those who dig the history a year down the
> road.

Information that nobody has requested but Johannes.

> Contributors who have _zero_ motiviation to find out that
> information are not welcome here because they cause friction and take
> away time from many others for _zero_ gain.

Fine, stay with the broken code.

Cheers.

--
Felipe Contreras

Re: [PATCH] test-lib: avoid full path to store test results

[Jeff King]

On Wed, Oct 31, 2012 at 07:02:48PM +0100, Johannes Sixt wrote:

> Am 31.10.2012 03:28, schrieb Felipe Contreras:
> > On Wed, Oct 31, 2012 at 3:13 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> >> Felipe Contreras wrote:
> >>
> >>> It's all fun and games to write explanations for things, but it's not
> >>> that easy when you want those explanations to be actually true, and
> >>> corrent--you have to spend time to make sure of that.
> >>
> >> That's why it's useful for the patch submitter to write them, asking
> >> for help when necessary.
> >>
> >> As a bonus, it helps reviewers understand the effect of the patch.
> >> Bugs averted!
> > 
> > Yeah, that would be nice. Too bad I don't have that information, and
> > have _zero_ motivation to go and get it for you.
> 
> Just to clarify: That information is not just for Jonathan, but for
> everyone on this list and those who dig the history a year down the
> road. Contributors who have _zero_ motiviation to find out that
> information are not welcome here because they cause friction and take
> away time from many others for _zero_ gain.

And me, who is trying to figure out what to do with this patch. It is
presented on its own, outside of a series, with only the description "no
reason not to do this". But AFAICT, it is _required_ for the tests in
the remote-hg series to work. Isn't that kind of an important
motivation?

Yet it is not in the commit message, nor does the remote-hg series
indicate that it should be built on top. Or am I wrong that the one is
dependent on the other?

-Peff

Re: [PATCH] test-lib: avoid full path to store test results

[Felipe Contreras]

On Fri, Nov 2, 2012 at 2:17 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Oct 31, 2012 at 07:02:48PM +0100, Johannes Sixt wrote:
>
>> Am 31.10.2012 03:28, schrieb Felipe Contreras:
>> > On Wed, Oct 31, 2012 at 3:13 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> >> Felipe Contreras wrote:
>> >>
>> >>> It's all fun and games to write explanations for things, but it's not
>> >>> that easy when you want those explanations to be actually true, and
>> >>> corrent--you have to spend time to make sure of that.
>> >>
>> >> That's why it's useful for the patch submitter to write them, asking
>> >> for help when necessary.
>> >>
>> >> As a bonus, it helps reviewers understand the effect of the patch.
>> >> Bugs averted!
>> >
>> > Yeah, that would be nice. Too bad I don't have that information, and
>> > have _zero_ motivation to go and get it for you.
>>
>> Just to clarify: That information is not just for Jonathan, but for
>> everyone on this list and those who dig the history a year down the
>> road. Contributors who have _zero_ motiviation to find out that
>> information are not welcome here because they cause friction and take
>> away time from many others for _zero_ gain.
>
> And me, who is trying to figure out what to do with this patch. It is
> presented on its own, outside of a series, with only the description "no
> reason not to do this".

Yeah, because I think it stands on its own. But I'll include it in the
remote-hg patch series, I already have it queued up.

> But AFAICT, it is _required_ for the tests in
> the remote-hg series to work. Isn't that kind of an important
> motivation?

It's not _requied_, you will see that error at the end, and the
aggregate results would all be 0, but the tests would still work.

> Yet it is not in the commit message, nor does the remote-hg series
> indicate that it should be built on top. Or am I wrong that the one is
> dependent on the other?

They are not dependent.

Cheers.

-- 
Felipe Contreras

Re: [PATCH] test-lib: avoid full path to store test results

[Jeff King]

On Fri, Nov 02, 2012 at 04:17:27PM +0100, Felipe Contreras wrote:

> > And me, who is trying to figure out what to do with this patch. It is
> > presented on its own, outside of a series, with only the description "no
> > reason not to do this".
> 
> Yeah, because I think it stands on its own. But I'll include it in the
> remote-hg patch series, I already have it queued up.

Thanks.

-Peff