tests: stop using shlex.split() and use lists instead

Stop using `shlex.split()` in conftest.py as a way to prepare test
commands for subprocess.run() because it does not always work. When we
have quoting issues, just avoid them entirely by passing the test
command as a list.

Replace shlex.split() with a simple str.split() for convenience in
simple cases. This simple split() will immediately fail when trying to
use quote which is working exactly as intended.

This also makes the cmd() interface more similar to subprocess.run() and
its many friends, which is good thing.

This is similar to commit 4100764 ("Project.git(list/str): reduce
reliance on shlex.split()") but applied to tests/

Before commit 624880e, shlex.split() was used unconditionally in
conftest.py. As expected, this eventually broke on Windows: shlex is
not compatible across all operating systems and shells.

https://docs.python.org/3/library/subprocess.html#security-considerations

> If the shell is invoked explicitly, via shell=True, it is the
> application’s responsibility to ensure that all whitespace and
> metacharacters are quoted appropriately to avoid shell injection
> vulnerabilities. On _some_ platforms, it is possible to use shlex.quote()
> for this escaping.

(Emphasis mine)

Then commit 624880e made the "interesting" decision to stop using
shlex.split()... only on Windows. This was discussed in
zephyrproject-rtos#266 (comment)

So after this commit, parsing of test commands was delegated to the
shell but... only on Windows! This worked for a long time but eventually
broke testing white spaces for the new `west alias` zephyrproject-rtos#716. Rather than
making that Windows-specific hack even more complex with a special case,
finally do the right thing and ask more complex test commands to use a
list. Now we can drop shlex.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>

tests/conftest.py

@@ -5,7 +5,6 @@
 import os
 from pathlib import Path, PurePath
 import platform
-import shlex
 import shutil
 import subprocess
 import sys
@@ -227,7 +226,7 @@ def west_init_tmpdir(repos_tmpdir):
     py.path.local, with the current working directory set there.'''
     west_tmpdir = repos_tmpdir / 'workspace'
     manifest = repos_tmpdir / 'repos' / 'zephyr'
-    cmd(f'init -m "{manifest}" "{west_tmpdir}"')
+    cmd(['init', '-m', str(manifest), str(west_tmpdir)])
     west_tmpdir.chdir()
     config.read_config()
     return west_tmpdir
@@ -327,9 +326,9 @@ def cmd(cmd, cwd=None, stderr=None, env=None):
     # stdout from cmd is captured and returned. The command is run in
     # a python subprocess so that program-level setup and teardown
     # happen fresh.
-    cmd = 'west ' + cmd
-    if not WINDOWS:
-        cmd = shlex.split(cmd)
+
+    cmd = ['west'] + (cmd.split() if isinstance(cmd, str) else cmd)
+
     print('running:', cmd)
     if env:
         print('with non-default environment:')

tests/test_project.py

@@ -77,6 +77,10 @@ def west_update_tmpdir(west_init_tmpdir):
 # Test cases
 #
 
+def _list_f(format):
+    return ['list', '-f', format]
+
+
 def test_workspace(west_update_tmpdir):
     # Basic test that west_update_tmpdir bootstrapped correctly. This
     # is a basic test of west init and west update.
@@ -104,7 +108,7 @@ def test_workspace(west_update_tmpdir):
 def test_list(west_update_tmpdir):
     # Projects shall be listed in the order they appear in the manifest.
     # Check the behavior for some format arguments of interest as well.
-    actual = cmd('list -f "{name} {revision} {path} {cloned} {clone_depth}"')
+    actual = cmd(_list_f('{name} {revision} {path} {cloned} {clone_depth}'))
     expected = ['manifest HEAD zephyr cloned None',
                 'Kconfiglib zephyr subdir/Kconfiglib cloned None',
                 'tagged_repo v1.0 tagged_repo cloned None',
@@ -116,16 +120,16 @@ def test_list(west_update_tmpdir):
     klib_rel = os.path.join('subdir', 'Kconfiglib')
     klib_abs = str(west_update_tmpdir.join('subdir', 'Kconfiglib'))
 
-    rel_outside = cmd(f'list -f "{{name}}" {klib_rel}').strip()
+    rel_outside = cmd(_list_f('{name}') + [klib_rel]).strip()
     assert rel_outside == 'Kconfiglib'
 
-    abs_outside = cmd(f'list -f "{{name}}" {klib_abs}').strip()
+    abs_outside = cmd(_list_f('{name}') + [klib_abs]).strip()
     assert abs_outside == 'Kconfiglib'
 
-    rel_inside = cmd('list -f "{name}" .', cwd=klib_abs).strip()
+    rel_inside = cmd('list -f {name} .', cwd=klib_abs).strip()
     assert rel_inside == 'Kconfiglib'
 
-    abs_inside = cmd(f'list -f "{{name}}" {klib_abs}', cwd=klib_abs).strip()
+    abs_inside = cmd(_list_f('{name}') + [klib_abs], cwd=klib_abs).strip()
     assert abs_inside == 'Kconfiglib'
 
     with pytest.raises(subprocess.CalledProcessError):
@@ -143,19 +147,19 @@ def test_list_manifest(west_update_tmpdir):
     shutil.copy('zephyr/west.yml', 'manifest_moved/west.yml')
     cmd('config manifest.path manifest_moved')
 
-    path = cmd('list -f "{path}" manifest').strip()
-    abspath = cmd('list -f "{abspath}" manifest').strip()
-    posixpath = cmd('list -f "{posixpath}" manifest').strip()
+    path = cmd('list -f {path} manifest').strip()
+    abspath = cmd('list -f {abspath} manifest').strip()
+    posixpath = cmd('list -f {posixpath} manifest').strip()
     assert path == 'manifest_moved'
     assert Path(abspath) == west_update_tmpdir / 'manifest_moved'
     assert posixpath == Path(west_update_tmpdir).as_posix() + '/manifest_moved'
 
     path = cmd('list --manifest-path-from-yaml '
-               '-f "{path}" manifest').strip()
+               '-f {path} manifest').strip()
     abspath = cmd('list --manifest-path-from-yaml '
-                  '-f "{abspath}" manifest').strip()
+                  '-f {abspath} manifest').strip()
     posixpath = cmd('list --manifest-path-from-yaml '
-                    '-f "{posixpath}" manifest').strip()
+                    '-f {posixpath} manifest').strip()
     assert path == 'zephyr'
     assert Path(abspath) == Path(str(west_update_tmpdir / 'zephyr'))
     assert posixpath == Path(west_update_tmpdir).as_posix() + '/zephyr'
@@ -187,29 +191,29 @@ def check(command_string, expected):
         out_lines = cmd(command_string).splitlines()
         assert out_lines == expected
 
-    check('list -f "{name} .{groups}. {path}"',
+    check(_list_f('{name} .{groups}. {path}'),
           ['manifest .. zephyr',
            'bar .. path-for-bar'])
 
-    check('list -f "{name} .{groups}. {path}" foo',
+    check(_list_f('{name} .{groups}. {path}') + ['foo'],
           ['foo .foo-group-1,foo-group-2. foo'])
 
-    check('list -f "{name} .{groups}. {path}" baz',
+    check(_list_f('{name} .{groups}. {path}') + ['baz'],
           ['baz .baz-group. baz'])
 
-    check('list -f "{name} .{groups}. {path}" foo bar baz',
+    check(_list_f("{name} .{groups}. {path}") + 'foo bar baz'.split(),
           ['foo .foo-group-1,foo-group-2. foo',
            'bar .. path-for-bar',
            'baz .baz-group. baz'])
 
-    check('list --all -f "{name} .{groups}. {path}"',
+    check(_list_f('{name} .{groups}. {path}') + ['--all'],
           ['manifest .. zephyr',
            'foo .foo-group-1,foo-group-2. foo',
            'bar .. path-for-bar',
            'baz .baz-group. baz'])
 
     cmd('config manifest.group-filter +foo-group-1')
-    check('list -f "{name} .{groups}. {path}"',
+    check(_list_f('{name} .{groups}. {path}'),
           ['manifest .. zephyr',
            'foo .foo-group-1,foo-group-2. foo',
            'bar .. path-for-bar'])
@@ -219,7 +223,7 @@ def test_list_sha(west_update_tmpdir):
     # Regression test for listing with {sha}. This should print N/A
     # for the first project, which is the ManifestProject.
 
-    assert cmd('list -f "{sha}"').startswith("N/A")
+    assert cmd('list -f {sha}').startswith("N/A")
 
 
 def test_manifest_freeze(west_update_tmpdir):