tests: the cd-level issue

[Adrian Buehlmann]

On 2012-06-12 03:37, Mads Kiilerich wrote:
> Adrian Buehlmann wrote, On 06/11/2012 01:12 PM:
>> On 2012-06-11 12:45, Mads Kiilerich wrote:
>>> On 11/06/12 11:43, Adrian Buehlmann wrote:
>>>> On 2012-06-11 11:21, Pierre-Yves David wrote:
>>>>> If you are creating a new alias for cd; why not just add an explicite
>>>>> check and abortion when getting out of the scope ?
>>> ...
>>>
>>>> diff --git a/tests/run-tests.py b/tests/run-tests.py
>>>> --- a/tests/run-tests.py
>>>> +++ b/tests/run-tests.py
>>>> @@ -659,6 +659,9 @@
>>>>                prepos = pos
>>>>                pos = n
>>>>                addsalt(n, False)
>>>> +            cmd = l[4:].split()
>>>> +            if len(cmd) == 2 and cmd[0] == 'cd':
>>>> +                l = '  $ cd %s || exit 1\n' % cmd[1]
>>>>                script.append(l[4:])
>>>>            elif l.startswith('>  '): # continuations
>>>>                after.setdefault(prepos, []).append(l)
>>>> This only exits if the cd fails, but that's certainly an improvement. If
>>>> a cd fails, the test should better not proceed creating artifacts, so
>>>> that's a nice change.
>>>>
>>>> FWIW, this change doesn't help with cd .. going too high up though.
>>> It is simple to extend it to validating that the cd doesn't "escape". I
>>> did that to detect some problems ... or to prove that there were no
>>> problems left.
>> That check you inserted only triggers, if the cd command tries to cd
>> into directory that doesn't exist.
>>
>> So, that check is IMHO no prove that the are no cd-level problems left.
> 
> Correct.
> 
> tests: verify that 'cd' never go outside $TESTTMP
> 
> diff --git a/tests/run-tests.py b/tests/run-tests.py
> --- a/tests/run-tests.py
> +++ b/tests/run-tests.py
> @@ -661,7 +661,9 @@
>               addsalt(n, False)
>               cmd = l[4:].split()
>               if len(cmd) == 2 and cmd[0] == 'cd':
> -                l = '  $ cd %s || exit 1\n' % cmd[1]
> +                l = ('  $ cd %s || exit 1; '
> +                     'case %s in $PWD/*) echo escaped to $PWD;; esac\n'
> +                     % (cmd[1], wd))
>               script.append(l[4:])
>           elif l.startswith('  > '): # continuations
>               after.setdefault(prepos, []).append(l)
> 
> tests: verify that tests end up in $TESTTMP
> 
> diff --git a/tests/run-tests.py b/tests/run-tests.py
> --- a/tests/run-tests.py
> +++ b/tests/run-tests.py
> @@ -683,6 +683,8 @@
>       if skipping is not None:
>           after.setdefault(pos, []).append('  !!! missing #endif\n')
>       addsalt(n + 1, False)
> +    script.append('if [ "$PWD" != "%s" ]; then echo $PWD; fi\n' % wd)
> +    addsalt(n + 2, False)
> 
>       # Write out the script and execute it
>       fd, name = tempfile.mkstemp(suffix='hg-tst')
> 
>>
>>> That is however a runtime check just to check for a
>>> static 'programming' error in the test. I don't think that's worth it.
>> Well, I think such a check could be useful. If you don't think it is
>> worth it, then I'm asking a bit why the tests got into a state that
>> requred a cleanup like you did here:
>>
>> http://hg.intevation.org/mercurial/crew/rev/6ef3107c661e
> 
> Running a quick test with a local hack 'on demand' is IMO good enough 
> for something like this. You are welcome to clean it up and make it 
> cross platform ... and make a control mechanism or a commit description 
> that justify that it is worth running all the time.

With regards to the aspect of "worth running all the time":

How about adding an option to run-tests.py which is off by default? So
that the price for that check wouldn't have to be paid each time the
testsuite is run?

Perhaps (run-tests.py --help):

  --cdcheck             validate cd commands

Not everyone is that good a doing local hacks on demand. Also, If you
alreday have cooked a local hack, it could perhaps be preserved into
run-tests.py.