Fix testing subdir usage

Various tests called subdir several times with the same directory,
which is pointless.  In previous code, this emitted a message
though it did not fail.  Stop doing that.

The dir_fixture() method did some convoluted things to make sure
the directories to write to exist, change this around to simplify.
The subdir() method already combines the testdir, so dir_fixture
doesn't need to.  Also handle more cleanly the case of the target
directory being an absolute path, which seems to have been intended,
but would not work.

Also clean up file_fixture() along the same principles.

Change the subdir() method to not need to be given an ordered
list of directories; instead call os.makedirs on each so
intermediate steps are made automatically.  Along the way,
the print about making a directory that already existed vanishes.

Tests which did os.path.join on directory pieces when calling
file_fixture no longer do so, since file_fixture (and dir_fixture)
do joining on an arg which is a list of paths like many other scons
functions.

The testing doc was updated to fix some wording and reflect
the above changes.

Signed-off-by: Mats Wichmann <mats@linux.com>

3 files changed, 121 insertions, 104 deletions

diff --git a/testing/framework/TestCmd.py b/testing/framework/TestCmd.py
index 613f0e4..f0051ba 100644
--- a/testing/framework/TestCmd.py
+++ b/testing/framework/TestCmd.py
@@ -1326,23 +1326,23 @@ class TestCmd:
         return result
 
     def dir_fixture(self, srcdir, dstdir=None):
-        """ Copies the contents of the fixture dir to the test dir.
+        """ Copies the contents of the fixture directory to the test directory.
 
         If srcdir is an absolute path, it is tried directly, else
-        the fixture_dirs are prepended to it and tried in succession.
-        To tightly control the search order, the harness can be called
-        with FIXTURE_DIRS also including the test source directory
-        in the desired place, it will otherwise be tried last.
+        the fixture_dirs are searched in order to find the named fixture
+        directory.  To tightly control the search order, the harness may
+        be called with FIXTURE_DIRS set including the test source directory
+        in the desired position, else it will be tried last.
 
-        srcdir may be a list, in which case the elements are first
-        joined into a pathname.
+        If dstdir not an absolute path, it is taken as a destination under
+        the working dir (if omitted of the default None indicates '.',
+        aka the test dir).  dstdir is created automatically if needed.
 
-        If a dstdir is supplied, it is taken to be under the temporary
-        working dir.  dstdir is created automatically if needed.
+        srcdir or dstdir may be a list, in which case the elements are first
+        joined into a pathname.
         """
         if is_List(srcdir):
             srcdir = os.path.join(*srcdir)
-
         spath = srcdir
         if srcdir and self.fixture_dirs and not os.path.isabs(srcdir):
             for dir in self.fixture_dirs:
@@ -1352,20 +1352,18 @@ class TestCmd:
             else:
                 spath = srcdir
 
-        if dstdir:
-            dstdir = self.canonicalize(dstdir)
+        if not dstdir or dstdir == '.':
+            dstdir = self.workdir
         else:
-            dstdir = '.'
-
-        if dstdir != '.' and not os.path.exists(dstdir):
-            dstlist = self.parse_path(dstdir)
-            if len(dstlist) > 0 and dstlist[0] == ".":
-                dstlist = dstlist[1:]
-            for idx in range(len(dstlist)):
-                self.subdir(dstlist[:idx + 1])
-
-        if dstdir and self.workdir:
-            dstdir = os.path.join(self.workdir, dstdir)
+            if is_List(dstdir):
+                dstdir = os.path.join(*dstdir)
+            if os.path.isabs(dstdir):
+                os.makedirs(dstdir, exist_ok=True)
+            else:
+                dstlist = self.parse_path(dstdir)
+                if dstlist and dstlist[0] == ".":
+                    dstdir = os.path.join(dstlist[1:])
+                self.subdir(dstdir)
 
         for entry in os.listdir(spath):
             epath = os.path.join(spath, entry)
@@ -1377,20 +1375,21 @@ class TestCmd:
                 shutil.copy(epath, dpath)
 
     def file_fixture(self, srcfile, dstfile=None):
-        """ Copies a fixture file to the test dir, optionally renaming.
+        """ Copies a fixture file to the test directory, optionally renaming.
 
         If srcfile is an absolute path, it is tried directly, else
-        the fixture_dirs are prepended to it and tried in succession.
-        To tightly control the search order, the harness can be called
-        with FIXTURE_DIRS also including the test source directory
+        the fixture_dirs are searched in order to find the named fixture
+        file.  To tightly control the search order, the harness may
+        be called with FIXTURE_DIRS also including the test source directory
         in the desired place, it will otherwise be tried last.
 
-        srcfile may be a list, in which case the elements are first
-        joined into a pathname.
+        dstfile is the name to give the copied file; if the argument
+        is omitted the basename of srcfile is used. If dstfile is not
+        an absolute path name.  Any directory components of dstfile are
+        created automatically if needed.
 
-        dstfile is assumed to be under the temporary working directory
-        unless it is an absolute path name.  Any directory components
-        of dstfile are created automatically if needed.
+        srcfile or dstfile may be a list, in which case the elements are first
+        joined into a pathname.
         """
         if is_List(srcfile):
             srcfile = os.path.join(*srcfile)
@@ -1411,15 +1410,21 @@ class TestCmd:
             else:
                 return
         else:
-            dstpath, dsttail = os.path.split(dstfile)
-            if dstpath:
-                if not os.path.exists(os.path.join(self.workdir, dstpath)):
-                    dstlist = self.parse_path(dstpath)
-                    if len(dstlist) > 0 and dstlist[0] == ".":
-                        dstlist = dstlist[1:]
-                    for idx in range(len(dstlist)):
-                        self.subdir(dstlist[:idx + 1])
-            dpath = os.path.join(self.workdir, dstfile)
+            dstdir, dsttail = os.path.split(dstfile)
+            if dstdir:
+                # if dstfile has a dir part, and is not abspath, create
+                if os.path.isabs(dstdir):
+                    os.makedirs(dstdir, exist_ok=True)
+                    dpath = dstfile
+                else:
+                    dstlist = self.parse_path(dstdir)
+                    if dstlist and dstlist[0] == ".":
+                        # strip leading ./ if present
+                        dstdir = os.path.join(dstlist[1:])
+                    self.subdir(dstdir)
+                    dpath = os.path.join(self.workdir, dstfile)
+            else:
+                dpath = os.path.join(self.workdir, dstfile)
 
         shutil.copy(spath, dpath)
 
@@ -1653,13 +1658,11 @@ class TestCmd:
         """Creates new subdirectories under the temporary working directory.
 
         Creates a subdir for each argument.  An argument may be a list,
-        in which case the list elements are concatenated using the
-        os.path.join() method.  Subdirectories multiple levels deep
-        must be created using a separate argument for each level:
+        in which case the list elements are joined into a path.
 
-            test.subdir('sub', ['sub', 'dir'], ['sub', 'dir', 'ectory'])
-
-        Returns the number of subdirectories actually created.
+        Returns the number of directories created, not including
+        intermediate directories, for historical reasons.  A directory
+        which already existed is counted as "created".
         """
         count = 0
         for sub in subdirs:
@@ -1669,14 +1672,15 @@ class TestCmd:
                 sub = os.path.join(*sub)
             new = os.path.join(self.workdir, sub)
             try:
-                os.mkdir(new)
+                # okay to exist, we just do this for counting
+                os.makedirs(new, exist_ok=True)
+                count = count + 1
             except OSError as e:
-                print("Got error creating dir: %s :%s" % (sub, e))
                 pass
-            else:
-                count = count + 1
+
         return count
 
+
     def symlink(self, target, link):
         """Creates a symlink to the specified target.
 
diff --git a/testing/framework/TestCmdTests.py b/testing/framework/TestCmdTests.py
index 730f4a1..7017d2c 100644
--- a/testing/framework/TestCmdTests.py
+++ b/testing/framework/TestCmdTests.py
@@ -2725,33 +2725,46 @@ sys.stderr.write("run2 STDERR second line\\n")
         assert output == "run1 STDOUT ['foo', 'bar']\nrun1 STDOUT second line\n", output
 
 
-
 class subdir_TestCase(TestCmdTestCase):
     def test_subdir(self):
         """Test subdir()"""
-        test = TestCmd.TestCmd(workdir = '', subdir = ['no', 'such', 'subdir'])
-        assert not os.path.exists(test.workpath('no'))
+        # intermediate directories are created
+        test = TestCmd.TestCmd(workdir='', subdir=['no', 'such', 'subdir'])
+        assert os.path.exists(test.workpath('no'))
 
-        test = TestCmd.TestCmd(workdir = '', subdir = 'foo')
-        assert test.subdir('bar') == 1
-        assert test.subdir(['foo', 'succeed']) == 1
-        if os.name != "nt":
-            os.chmod(test.workpath('foo'), 0o500)
-            assert test.subdir(['foo', 'fail']) == 0
-        assert test.subdir(['sub', 'dir', 'ectory'], 'sub') == 1
-        assert test.subdir('one',
-                           UserList(['one', 'two']),
-                           ['one', 'two', 'three']) == 3
+        test = TestCmd.TestCmd(workdir='', subdir='foo')
         assert os.path.isdir(test.workpath('foo'))
+
+        # single subdir
+        assert test.subdir('bar')
         assert os.path.isdir(test.workpath('bar'))
+
+        # subdir "works" even if existing
+        assert test.subdir('bar')
+
+        # single subdir as a list
+        assert test.subdir(['foo', 'succeed'])
         assert os.path.isdir(test.workpath('foo', 'succeed'))
+
         if os.name != "nt":
             assert not os.path.exists(test.workpath('foo', 'fail'))
+
+        # subdir creation without write permissions fails
+        if os.name != "nt":
+            os.chmod(test.workpath('foo'), 0o500)
+            assert not test.subdir(['foo', 'fail'])
+
+        # create descended path
+        assert test.subdir(['sub', 'dir', 'ectory'])
         assert os.path.isdir(test.workpath('sub'))
-        assert not os.path.exists(test.workpath('sub', 'dir'))
-        assert not os.path.exists(test.workpath('sub', 'dir', 'ectory'))
-        assert os.path.isdir(test.workpath('one', 'two', 'three'))
+        assert os.path.exists(test.workpath('sub', 'dir'))
+        assert os.path.exists(test.workpath('sub', 'dir', 'ectory'))
 
+        # test multiple subdirs in one call, each should "succeed"
+        assert (
+            test.subdir('one', UserList(['one', 'two']), ['one', 'two', 'three']) == 3
+        )
+        assert os.path.isdir(test.workpath('one', 'two', 'three'))
 
 
 class symlink_TestCase(TestCmdTestCase):
diff --git a/testing/framework/test-framework.rst b/testing/framework/test-framework.rst
index 8cac1fb..dca4684 100644
--- a/testing/framework/test-framework.rst
+++ b/testing/framework/test-framework.rst
@@ -335,9 +335,9 @@ The test harness method ``dir_fixture(srcdir, [dstdir])``
 copies the contents of the specified directory ``srcdir`` from
 the directory of the called test script to the current temporary test
 directory.  The ``srcdir`` name may be a list, in which case the elements
-are concatenated into a path first.  The ``dstdir``
-is assumed to be under the temporary working directory, it gets created
-automatically, if it does not already exist.
+are concatenated into a path first.  The optional ``dstdir`` is
+used as a destination path under the temporary working directory.
+``distdir`` is created automatically, if it does not already exist.
 
 If ``srcdir`` represents an absolute path, it is used as-is.
 Otherwise, if the harness was invoked with the environment variable
@@ -361,14 +361,15 @@ To see a real example for this in action, refer to the test named
 File fixtures
 -------------
 
-Similarly, the method ``file_fixture(srcfile, [dstfile])``
-copies the file ``srcfile`` from the directory of the called script,
-to the temporary test directory.  The ``srcfile`` name may be a list,
-in which case the elements are concatenated into a path first.
-The ``dstfile`` is assumed to be under the temporary working directory,
-unless it is an absolute path name.
-If ``dstfile`` is specified, its target directory gets created
-automatically if it doesn't already exist.
+The method ``file_fixture(srcfile, [dstfile])``
+copies the file ``srcfile`` from the directory of the called script
+to the temporary test directory.
+The optional ``dstfile`` is used as a destination file name
+under the temporary working directory, unless it is an absolute path name.
+If ``dstfile`` includes directory elements, they are
+created automatically if they don't already exist.
+The ``srcfile`` and ``dstfile`` parameters may each be a list,
+which will be concatenated into a path.
 
 If ``srcfile`` represents an absolute path, it is used as-is. Otherwise,
 any passed in fixture directories are used as additional places to
@@ -378,16 +379,16 @@ With the following code::
 
    test = TestSCons.TestSCons()
    test.file_fixture('SConstruct')
-   test.file_fixture(['src','main.cpp'],['src','main.cpp'])
+   test.file_fixture(['src', 'main.cpp'], ['src', 'main.cpp'])
    test.run()
 
 The files ``SConstruct`` and ``src/main.cpp`` are copied to the
-temporary test directory. Notice the second ``file_fixture`` line
+temporary test directory. Notice the second ``file_fixture`` call
 preserves the path of the original, otherwise ``main.cpp``
 would have been placed in the top level of the test directory.
 
 Again, a reference example can be found in the current revision
-of SCons, it is ``test/packaging/sandbox-test/sandbox-test.py``.
+of SCons, see ``test/packaging/sandbox-test/sandbox-test.py``.
 
 For even more examples you should check out
 one of the external Tools, e.g. the *Qt4* Tool at
@@ -401,12 +402,11 @@ How to convert old tests to use fixures
 Tests using the inline ``TestSCons.write()`` method can fairly easily be
 converted to the fixture based approach. For this, we need to get at the
 files as they are written to each temporary test directory,
-which we can do by taking advantage of a debugging aid:
-
-``runtest.py`` checks for the existence of an environment
+which we can do by taking advantage of an existing debugging aid,
+namely that ``runtest.py`` checks for the existence of an environment
 variable named ``PRESERVE``. If it is set to a non-zero value, the testing
 framework preserves the test directory instead of deleting it, and prints
-its name to the screen.
+a message about its name to the screen.
 
 So, you should be able to give the commands::
 
@@ -418,29 +418,27 @@ assuming Linux and a bash-like shell. For a Windows ``cmd`` shell, use
 
 The output will then look something like this::
 
-   1/1 (100.00%) /usr/bin/python -tt test/packaging/sandbox-test.py
-   pASSED
+   1/1 (100.00%) /usr/bin/python test/packaging/sandbox-test.py
+   PASSED
    preserved directory /tmp/testcmd.4060.twlYNI
 
 You can now copy the files from that directory to your new
 *fixture* directory. Then, in the test script you simply remove all the
-tedious ``TestSCons.write()`` statements and replace them by a single
-``TestSCons.dir_fixture()``.
-
-Finally, don't forget to clean up and remove the temporary test
-directory. ``;)``
+tedious ``TestSCons.write()`` statements and replace them with a single
+``TestSCons.dir_fixture()`` call.
 
 For more complex testing scenarios you can use ``file_fixture`` with
-the option to rename (that is, supplying a second argument, which is
-the name to give the fixture file being copied).  For example some test
-files write multiple ``SConstruct`` files across the full run.
-These files can be given different names - perhaps using a sufffix -
-and then sucessively copied to the final name as needed::
+the optional second argument (or the keyword arg ``dstfile``) to assign
+a name to the file being copied.  For example, some tests need to
+write multiple ``SConstruct`` files across the full run.
+These files can be given different names in the source (perhaps using a 
+sufffix to distinguish them), and then be sucessively copied to the
+final name as needed::
 
    test.file_fixture('fixture/SConstruct.part1', 'SConstruct')
    # more setup, then run test
    test.file_fixture('fixture/SConstruct.part2', 'SConstruct')
-   # etc.
+   # run new test
 
 
 When not to use a fixture
@@ -457,11 +455,13 @@ kind of usage that does not lend itself to a fixture::
 
    test.write('SConstruct', """
    cc = Environment().Dictionary('CC')
-   env = Environment(LINK=r'%(_python_)s mylink.py',
-                     LINKFLAGS=[],
-                     CC=r'%(_python_)s mycc.py',
-                     CXX=cc,
-                     CXXFLAGS=[])
+   env = Environment(
+       LINK=r'%(_python_)s mylink.py',
+       LINKFLAGS=[],
+       CC=r'%(_python_)s mycc.py',
+       CXX=cc,
+       CXXFLAGS=[],
+   )
    env.Program(target='test1', source='test1.c')
    """ % locals())
 
@@ -469,7 +469,7 @@ Here the value of ``_python_`` is picked out of the script's
 ``locals`` dictionary - which works because we've set it above -
 and interpolated using a mapping key into the string that will
 be written to ``SConstruct``. A fixture would be hard to use
-here because we don't know the value of `_python_` until runtime.
+here because we don't know the value of ``_python_`` until runtime.
 
 The other files created in this test may still be candidates for
 use as fixture files, however.