tests: the cd-level issue

[Mads Kiilerich]

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.

/Mads