Re: [PATCH] test-lib: avoid full path to store test results
On Wed, Oct 31, 2012 at 2:27 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 19 quoted lines
> 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.
Show 5 quoted lines
> $ ~/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.
Show 7 quoted lines
> 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
Felipe Contreras
