From 380a486a512cfebb4aef8ff09d9710543b419be9 Mon Sep 17 00:00:00 2001 From: Marc Herbert Date: Wed, 28 Aug 2024 19:49:58 +0000 Subject: [PATCH] tests: stop using shlex.split() and use lists instead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 41007648d993 ("Project.git(list/str): reduce reliance on shlex.split()") but applied to tests/ Before commit 624880e8ffd2, 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 624880e8ffd2 made the "interesting" decision to stop using shlex.split()... only on Windows. This was discussed in https://github.com/zephyrproject-rtos/west/pull/266#discussion_r284670076 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` #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 --- tests/conftest.py | 9 ++++----- tests/test_project.py | 40 ++++++++++++++++++++++------------------ 2 files changed, 26 insertions(+), 23 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 697ecb9b..2d88b8a0 100644 --- a/tests/conftest.py +++ b/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:') diff --git a/tests/test_project.py b/tests/test_project.py index 9cbc88f5..dfa21ac6 100644 --- a/tests/test_project.py +++ b/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):