From 41fbf25dd7a3ceff4fd06c49d5a359dd770f9782 Mon Sep 17 00:00:00 2001 From: Marti Bolivar Date: Thu, 2 May 2019 06:41:02 -0600 Subject: [PATCH 01/15] tests: flatten the hierarchy There's no good reason to have these tests in subdirectories. Flatten them out. Keep the directory of invalid manifests separate to keep the directory listing clean, though. Signed-off-by: Marti Bolivar --- tests/{west/commands => }/conftest.py | 6 +++--- tests/{west/commands => }/empty_config.py | 0 .../manifest => manifests}/invalid_bad_default_remote.yml | 0 .../invalid_duplicate_project_path.yml | 0 tests/{west/manifest => manifests}/invalid_empty.yml | 0 .../manifest => manifests}/invalid_ill_defined_project.yml | 0 .../manifest => manifests}/invalid_ill_defined_remote.yml | 0 tests/{west/manifest => manifests}/invalid_no_projects.yml | 0 tests/{west/manifest => manifests}/invalid_no_remotes.yml | 0 tests/{west/manifest => manifests}/invalid_too_flat.yml | 0 .../manifest => manifests}/invalid_west_commands_1.yml | 0 .../manifest => manifests}/invalid_west_commands_2.yml | 0 .../manifest => manifests}/invalid_west_commands_3.yml | 0 tests/{west/commands => }/test_config.py | 0 tests/{west/commands => }/test_help.py | 0 tests/{west/manifest => }/test_manifest.py | 3 ++- tests/{west/commands => }/test_project.py | 0 17 files changed, 5 insertions(+), 4 deletions(-) rename tests/{west/commands => }/conftest.py (98%) rename tests/{west/commands => }/empty_config.py (100%) rename tests/{west/manifest => manifests}/invalid_bad_default_remote.yml (100%) rename tests/{west/manifest => manifests}/invalid_duplicate_project_path.yml (100%) rename tests/{west/manifest => manifests}/invalid_empty.yml (100%) rename tests/{west/manifest => manifests}/invalid_ill_defined_project.yml (100%) rename tests/{west/manifest => manifests}/invalid_ill_defined_remote.yml (100%) rename tests/{west/manifest => manifests}/invalid_no_projects.yml (100%) rename tests/{west/manifest => manifests}/invalid_no_remotes.yml (100%) rename tests/{west/manifest => manifests}/invalid_too_flat.yml (100%) rename tests/{west/manifest => manifests}/invalid_west_commands_1.yml (100%) rename tests/{west/manifest => manifests}/invalid_west_commands_2.yml (100%) rename tests/{west/manifest => manifests}/invalid_west_commands_3.yml (100%) rename tests/{west/commands => }/test_config.py (100%) rename tests/{west/commands => }/test_help.py (100%) rename tests/{west/manifest => }/test_manifest.py (98%) rename tests/{west/commands => }/test_project.py (100%) diff --git a/tests/west/commands/conftest.py b/tests/conftest.py similarity index 98% rename from tests/west/commands/conftest.py rename to tests/conftest.py index cd0e744d..6e6e6146 100644 --- a/tests/west/commands/conftest.py +++ b/tests/conftest.py @@ -10,9 +10,9 @@ import pytest GIT = shutil.which('git') -# Assumes this file is west/tests/west/commands/conftest.py, returns -# path to toplevel 'west' -THIS_WEST = os.path.abspath(dirname(dirname(dirname(dirname(__file__))))) +# Assumes this file is west/tests/conftest.py, returns path to +# toplevel 'west' +THIS_WEST = os.path.abspath(dirname((dirname(__file__)))) # # Test fixtures diff --git a/tests/west/commands/empty_config.py b/tests/empty_config.py similarity index 100% rename from tests/west/commands/empty_config.py rename to tests/empty_config.py diff --git a/tests/west/manifest/invalid_bad_default_remote.yml b/tests/manifests/invalid_bad_default_remote.yml similarity index 100% rename from tests/west/manifest/invalid_bad_default_remote.yml rename to tests/manifests/invalid_bad_default_remote.yml diff --git a/tests/west/manifest/invalid_duplicate_project_path.yml b/tests/manifests/invalid_duplicate_project_path.yml similarity index 100% rename from tests/west/manifest/invalid_duplicate_project_path.yml rename to tests/manifests/invalid_duplicate_project_path.yml diff --git a/tests/west/manifest/invalid_empty.yml b/tests/manifests/invalid_empty.yml similarity index 100% rename from tests/west/manifest/invalid_empty.yml rename to tests/manifests/invalid_empty.yml diff --git a/tests/west/manifest/invalid_ill_defined_project.yml b/tests/manifests/invalid_ill_defined_project.yml similarity index 100% rename from tests/west/manifest/invalid_ill_defined_project.yml rename to tests/manifests/invalid_ill_defined_project.yml diff --git a/tests/west/manifest/invalid_ill_defined_remote.yml b/tests/manifests/invalid_ill_defined_remote.yml similarity index 100% rename from tests/west/manifest/invalid_ill_defined_remote.yml rename to tests/manifests/invalid_ill_defined_remote.yml diff --git a/tests/west/manifest/invalid_no_projects.yml b/tests/manifests/invalid_no_projects.yml similarity index 100% rename from tests/west/manifest/invalid_no_projects.yml rename to tests/manifests/invalid_no_projects.yml diff --git a/tests/west/manifest/invalid_no_remotes.yml b/tests/manifests/invalid_no_remotes.yml similarity index 100% rename from tests/west/manifest/invalid_no_remotes.yml rename to tests/manifests/invalid_no_remotes.yml diff --git a/tests/west/manifest/invalid_too_flat.yml b/tests/manifests/invalid_too_flat.yml similarity index 100% rename from tests/west/manifest/invalid_too_flat.yml rename to tests/manifests/invalid_too_flat.yml diff --git a/tests/west/manifest/invalid_west_commands_1.yml b/tests/manifests/invalid_west_commands_1.yml similarity index 100% rename from tests/west/manifest/invalid_west_commands_1.yml rename to tests/manifests/invalid_west_commands_1.yml diff --git a/tests/west/manifest/invalid_west_commands_2.yml b/tests/manifests/invalid_west_commands_2.yml similarity index 100% rename from tests/west/manifest/invalid_west_commands_2.yml rename to tests/manifests/invalid_west_commands_2.yml diff --git a/tests/west/manifest/invalid_west_commands_3.yml b/tests/manifests/invalid_west_commands_3.yml similarity index 100% rename from tests/west/manifest/invalid_west_commands_3.yml rename to tests/manifests/invalid_west_commands_3.yml diff --git a/tests/west/commands/test_config.py b/tests/test_config.py similarity index 100% rename from tests/west/commands/test_config.py rename to tests/test_config.py diff --git a/tests/west/commands/test_help.py b/tests/test_help.py similarity index 100% rename from tests/west/commands/test_help.py rename to tests/test_help.py diff --git a/tests/west/manifest/test_manifest.py b/tests/test_manifest.py similarity index 98% rename from tests/west/manifest/test_manifest.py rename to tests/test_manifest.py index 3ca66196..3fe44d38 100644 --- a/tests/west/manifest/test_manifest.py +++ b/tests/test_manifest.py @@ -267,7 +267,8 @@ def test_west_is_ok(): # Invalid manifests should raise MalformedManifest. @pytest.mark.parametrize('invalid', - glob(os.path.join(THIS_DIRECTORY, 'invalid_*.yml'))) + glob(os.path.join(THIS_DIRECTORY, 'manifests', + 'invalid_*.yml'))) @patch('west.util.west_topdir', return_value='/west_top') def test_invalid(topdir, invalid): with pytest.raises(MalformedManifest): diff --git a/tests/west/commands/test_project.py b/tests/test_project.py similarity index 100% rename from tests/west/commands/test_project.py rename to tests/test_project.py From 8d329beedb7c44f35fdfe93ab311062dc390eae0 Mon Sep 17 00:00:00 2001 From: Marti Bolivar Date: Tue, 14 May 2019 09:45:37 -0600 Subject: [PATCH 02/15] tests: remove empty_config.py It's not clear what this is doing here. It doesn't begin with "test", so it's not being tested, and it seems to be a copy of test_config.py with some important features needed to avoid modifying the user's configuration. Delete it. Signed-off-by: Marti Bolivar --- tests/empty_config.py | 134 ------------------------------------------ 1 file changed, 134 deletions(-) delete mode 100644 tests/empty_config.py diff --git a/tests/empty_config.py b/tests/empty_config.py deleted file mode 100644 index 8ba89dc3..00000000 --- a/tests/empty_config.py +++ /dev/null @@ -1,134 +0,0 @@ -# Copyright (c) 2019, Nordic Semiconductor ASA -# -# SPDX-License-Identifier: Apache-2.0 - -# -# Test cases -# -# For some tests cases, the environment setting HOME must be redirected to a -# tmp folder. -# This is configured in tox.ini to ensure tests can run without modifying the -# settings of the calling user. -# If running this test directly using pytest, those tests will be skipped. -# - -import os -import pytest -from west import configuration as config -from conftest import cmd - - -# We skip this test if executed directly using pytest, to avoid modifying -# user's real ~/.westconfig. -# We want to ensure HOME is pointing inside TOX temp dir before continuing. -@pytest.mark.skipif(os.environ.get('TOXTEMPDIR') is None, - reason="This test requires to be executed using tox") -def test_config_global(west_init_tmpdir): - if not os.path.exists(os.path.expanduser('~')): - os.mkdir(os.path.expanduser('~')) - - # To ensure that GLOBAL home folder points into tox temp dir. - # Otherwise fail the test, as we don't want to risk manipulating user's - # west config - assert config.ConfigFile.GLOBAL.value == \ - os.path.join(os.environ.get('TOXTEMPDIR'), 'pytest-home/.westconfig') - - testkey_value = cmd('config pytest.testkey_global') - assert testkey_value == '' - - # Set value in user's testing home - cmd('config --global pytest.testkey_global ' + str(west_init_tmpdir)) - - # Readback from --local, to ensure that is empty. - testkey_value = cmd('config --local pytest.testkey_global') - assert testkey_value == '' - - # Readback from --system, to ensure that is empty. - testkey_value = cmd('config --system pytest.testkey_global') - assert testkey_value == '' - - # Readback from --global, and compare the value with the expected. - testkey_value = cmd('config --global pytest.testkey_global') - assert testkey_value.strip('\n') == str(west_init_tmpdir) - - # Readback in from all files should also provide the value. - testkey_value = cmd('config pytest.testkey_global') - assert testkey_value.strip('\n') == str(west_init_tmpdir) - - -def test_config_local(west_init_tmpdir): - if not os.path.exists(os.path.expanduser('~')): - os.mkdir(os.path.expanduser('~')) - - test_value = str(west_init_tmpdir) + '_local' - - testkey_value = cmd('config pytest.testkey_local') - assert testkey_value == '' - - # Set value in project local - cmd('config --local pytest.testkey_local ' + test_value) - - # Readback from --global, to ensure that is empty. - testkey_value = cmd('config --global pytest.testkey_local') - assert testkey_value == '' - - # Readback from --local, and compare the value with the expected. - testkey_value = cmd('config --local pytest.testkey_local') - assert testkey_value.strip('\n') == test_value - - # Readback in from all files should also provide the value. - testkey_value = cmd('config pytest.testkey_local') - assert testkey_value.strip('\n') == test_value - - # Update the value in user's testing home without --local and see it's - # default to --local when reading back - test_value_update = str(west_init_tmpdir) + '_update' - cmd('config pytest.testkey_local ' + test_value_update) - - # Readback from --global, to ensure that is empty. - testkey_value = cmd('config --global pytest.testkey_local') - assert testkey_value == '' - - # Readback from --local, and compare the value with the expected. - testkey_value = cmd('config --local pytest.testkey_local') - assert testkey_value.strip('\n') == test_value_update - - # Readback in from all files should also provide the value. - testkey_value = cmd('config pytest.testkey_local') - assert testkey_value.strip('\n') == test_value_update - - -def test_config_precendence(west_init_tmpdir): - if not os.path.exists(os.path.expanduser('~')): - os.mkdir(os.path.expanduser('~')) - - test_value_local = str(west_init_tmpdir) + '_precedence' - test_value_global = str(west_init_tmpdir) + '_global' - - testkey_value = cmd('config pytest.testkey_precedence') - assert testkey_value == '' - - # Set value in user's testing home - cmd('config --global pytest.testkey_precedence ' + test_value_global) - - # Readback from --global, to verify it is set. - testkey_value = cmd('config --global pytest.testkey_precedence') - assert testkey_value.strip('\n') == test_value_global - - # Readback from --local, to ensure it is not available using --local - testkey_value = cmd('config --local pytest.testkey_precedence') - assert testkey_value.strip('\n') == '' - - # Set value in project local and verify it can be read back. - cmd('config --local pytest.testkey_precedence ' + test_value_local) - testkey_value = cmd('config --local pytest.testkey_precedence') - assert testkey_value.strip('\n') == test_value_local - - # Readback without specifying --local or --global and see that project - # specific value takes precedence. - testkey_value = cmd('config pytest.testkey_precedence') - assert testkey_value.strip('\n') == test_value_local - - # Make additional verification that --global is still available. - testkey_value = cmd('config --global pytest.testkey_precedence') - assert testkey_value.strip('\n') == test_value_global From 884170e6182b65bcc32b34059e9df5538e42b5ff Mon Sep 17 00:00:00 2001 From: Marti Bolivar Date: Tue, 14 May 2019 09:24:13 -0600 Subject: [PATCH 03/15] tests: don't test pre-unification features Don't copy the west tree; tox already installs it for us into the new virtualenv, and we don't run any code out of a checked out repository anymore, so doing things related to that is unnecessary. This also makes the tests run a little bit faster (around a 5% or more speedup on my system). Signed-off-by: Marti Bolivar --- tests/conftest.py | 66 ++--------------------------------------------- tox.ini | 4 --- 2 files changed, 2 insertions(+), 68 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 6e6e6146..da2e8d71 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,5 +1,4 @@ import os -from os.path import dirname import shlex import shutil import subprocess @@ -10,9 +9,6 @@ import pytest GIT = shutil.which('git') -# Assumes this file is west/tests/conftest.py, returns path to -# toplevel 'west' -THIS_WEST = os.path.abspath(dirname((dirname(__file__)))) # # Test fixtures @@ -32,8 +28,6 @@ def repos_tmpdir(tmpdir): with the following contents: repos/ - ├── west (branch: master) - │ └── (contains this west's worktree contents) ├── manifest (branch: master) │ └── west.yml ├── Kconfiglib (branch: zephyr) @@ -51,8 +45,6 @@ def repos_tmpdir(tmpdir): The contents of west.yml are: - west: - url: file:///west manifest: defaults: remote: test-local @@ -72,11 +64,6 @@ def repos_tmpdir(tmpdir): rr = tmpdir.mkdir('repos') # "remote" repositories rp = {} # individual repository paths under rr - # Mirror this west tree into a "remote" west repository under rr. - wdst = rr.join('west') - mirror_west_repo(wdst) - rp['west'] = str(wdst) - # Create the other repositories. for repo in 'net-tools', 'Kconfiglib', 'zephyr': path = str(rr.join(repo)) @@ -86,8 +73,6 @@ def repos_tmpdir(tmpdir): # Initialize the manifest repository. add_commit(rp['zephyr'], 'test manifest', files={'west.yml': textwrap.dedent('''\ - west: - url: file://{west} manifest: defaults: remote: test-local @@ -104,7 +89,7 @@ def repos_tmpdir(tmpdir): west-commands: scripts/west-commands.yml self: path: zephyr - '''.format(west=rp['west'], rr=str(rr)))}) + '''.format(rr=str(rr)))}) # Initialize the Kconfiglib repository. subprocess.check_call([GIT, 'checkout', '-b', 'zephyr'], @@ -160,8 +145,7 @@ def west_init_tmpdir(repos_tmpdir): Uses the remote repositories from the repos_tmpdir fixture to create a west installation using the system bootstrapper's init - command -- and thus the test environment must install the - bootstrapper from the current west source code tree under test. + command. The contents of the west installation aren't checked at all. This is left up to the test cases. @@ -194,52 +178,6 @@ def check_output(*args, **kwargs): raise return out_bytes.decode(sys.getdefaultencoding()) - -def mirror_west_repo(dst): - # Create a west repository in dst which mirrors the exact state of - # the current tree, except ignored files. - # - # This is done in a simple way: - # - # 1. recursively copy THIS_WEST there (except .git and ignored files) - # 2. init a new git repository there - # 3. add the entire tree, and commit - # - # (We can't just clone THIS_WEST because we want to allow - # developers to test their working trees without having to make a - # commit -- remember, 'west init' clones the remote.) - wut = str(dst) # "west under test" - - # Copy the west working tree, except ignored files. - def ignore(directory, files): - # Get newline separated list of ignored files, as a string. - try: - ignored = check_output([GIT, 'check-ignore'] + files, - cwd=directory) - except subprocess.CalledProcessError as e: - # From the manpage: return values 0 and 1 respectively - # mean that some and no argument files were ignored. These - # are both OK. Treat other return values as errors. - if e.returncode not in (0, 1): - raise - else: - ignored = e.output.decode(sys.getdefaultencoding()) - - # Convert ignored to a set of file names as strings. - ignored = set(ignored.splitlines()) - - # Also ignore the .git directory itself. - if '.git' in files: - ignored.add('.git') - - return ignored - shutil.copytree(THIS_WEST, wut, ignore=ignore) - - # Create a fresh .git and commit existing directory tree. - create_repo(wut) - subprocess.check_call([GIT, 'add', '-A'], cwd=wut) - add_commit(wut, 'west under test') - def cmd(cmd, cwd=None, stderr=None): # Run a west command in a directory (cwd defaults to os.getcwd()). # diff --git a/tox.ini b/tox.ini index 4a9dd0f7..b66bec49 100644 --- a/tox.ini +++ b/tox.ini @@ -19,11 +19,7 @@ platform = posix: (linux|darwin) windows: win32 whitelist_externals = py.test -# Tests which import west modules directly from src need this PYTHONPATH -# available. The sdist which tox builds and installs only contains the -# bootstrapper modules. setenv = - PYTHONPATH={toxinidir}/src # The config test will be using users HOME, therefore we redirect it # into tox temp dir during testing. HOME={envtmpdir}/pytest-home From 1cb9be1b189f638b99265d7c2d73033d73ab31c3 Mon Sep 17 00:00:00 2001 From: Marti Bolivar Date: Tue, 14 May 2019 13:46:08 -0600 Subject: [PATCH 04/15] tests: fixture setup optimizations It takes 20 seconds on my machine to run the full set of tests, which is slow enough that testing breaks me out of flow state. On the suspicion that creating git repositories and using the file:// protocol when cloning (which prevents use of hardlinks) is slowing things down, use some pytest features to avoid creating git repositories repeatedly. Also let git use hardlinks when they are available when cloning repositories. On my system, this brings the average of 10 runs from 20.129 seconds spent testing to 17.649, a 12% improvement overall. Still not ideal, but not worth throwing away, either. Signed-off-by: Marti Bolivar --- tests/conftest.py | 180 +++++++++++++++++++++++++--------------------- 1 file changed, 98 insertions(+), 82 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index da2e8d71..b1d963d9 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -9,87 +9,52 @@ import pytest GIT = shutil.which('git') +MANIFEST_TEMPLATE = '''\ +manifest: + defaults: + remote: test-local + + remotes: + - name: test-local + url-base: THE_URL_BASE + + projects: + - name: Kconfiglib + revision: zephyr + path: subdir/Kconfiglib + - name: net-tools + west-commands: scripts/west-commands.yml + self: + path: zephyr +''' # # Test fixtures # -@pytest.fixture -def repos_tmpdir(tmpdir): - '''Fixture for tmpdir with "remote" repositories, manifest, and west. - - These can then be used to bootstrap an installation and run - project-related commands on it with predictable results. - - Switches directory to, and returns, the top level tmpdir -- NOT - the subdirectory containing the repositories themselves. +@pytest.fixture(scope='session') +def _session_repos(): + '''Just a helper, do not use directly.''' - Initializes placeholder upstream repositories in /remote-repos/ - with the following contents: + # It saves time to create repositories once at session scope, then + # clone the results as needed in per-test fixtures. + session_repos = os.path.join(os.environ['TOXTEMPDIR'], 'session_repos') + print('initializing session repositories in', session_repos) + shutil.rmtree(session_repos, ignore_errors=True) - repos/ - ├── manifest (branch: master) - │ └── west.yml - ├── Kconfiglib (branch: zephyr) - │ └── kconfiglib.py - ├── net-tools (branch: master) - │ └── qemu-script.sh - └── zephyr (branch: master) - ├── CODEOWNERS - ├── west.yml - ├── include - │ └── header.h - └── subsys - └── bluetooth - └── code.c - - The contents of west.yml are: - - manifest: - defaults: - remote: test-local - remotes: - - name: test-local - url-base: file:///remote-repos - projects: - - name: Kconfiglib - revision: zephyr - path: subdir/Kconfiglib - - name: net-tools - clone_depth: 1 - west-commands: scripts/west-commands.yml - self: - path: zephyr - ''' - rr = tmpdir.mkdir('repos') # "remote" repositories - rp = {} # individual repository paths under rr - - # Create the other repositories. + # Create the repositories. + rp = {} # individual repository paths for repo in 'net-tools', 'Kconfiglib', 'zephyr': - path = str(rr.join(repo)) + path = os.path.join(session_repos, repo) rp[repo] = path create_repo(path) - # Initialize the manifest repository. - add_commit(rp['zephyr'], 'test manifest', - files={'west.yml': textwrap.dedent('''\ - manifest: - defaults: - remote: test-local - - remotes: - - name: test-local - url-base: file://{rr} - - projects: - - name: Kconfiglib - revision: zephyr - path: subdir/Kconfiglib - - name: net-tools - west-commands: scripts/west-commands.yml - self: - path: zephyr - '''.format(rr=str(rr)))}) + # Initialize the "zephyr" repository. + # The caller needs to add west.yml with the right url-base. + add_commit(rp['zephyr'], 'base zephyr commit', + files={'CODEOWNERS': '', + 'include/header.h': '#pragma once\n', + 'subsys/bluetooth/code.c': 'void foo(void) {}\n'}) # Initialize the Kconfiglib repository. subprocess.check_call([GIT, 'checkout', '-b', 'zephyr'], @@ -124,20 +89,69 @@ def do_run(self, args, ignored): '''), }) - # Initialize the zephyr repository. - add_commit(rp['zephyr'], 'test zephyr commit', - files={'CODEOWNERS': '', - 'include/header.h': '#pragma once\n', - 'subsys/bluetooth/code.c': 'void foo(void) {}\n'}) + # Return the top-level temporary directory. Don't clean it up on + # teardown, so the contents can be inspected post-portem. + print('finished initializing session repositories') + return session_repos - # Switch to and return the top-level temporary directory. - # - # This can be used to populate a west installation alongside. +@pytest.fixture +def repos_tmpdir(tmpdir, _session_repos): + '''Fixture for tmpdir with "remote" repositories, manifest, and west. - # Switch to the top-level West installation directory - tmpdir.chdir() - return tmpdir + These can then be used to bootstrap an installation and run + project-related commands on it with predictable results. + + Switches directory to, and returns, the top level tmpdir -- NOT + the subdirectory containing the repositories themselves. + + Initializes placeholder upstream repositories in tmpdir with the + following contents: + + repos/ + ├── Kconfiglib (branch: zephyr) + │ └── kconfiglib.py + ├── net-tools (branch: master) + │ └── qemu-script.sh + └── zephyr (branch: master) + ├── CODEOWNERS + ├── west.yml + ├── include + │ └── header.h + └── subsys + └── bluetooth + └── code.c + + The contents of west.yml are: + + manifest: + defaults: + remote: test-local + remotes: + - name: test-local + url-base: /repos + projects: + - name: Kconfiglib + revision: zephyr + path: subdir/Kconfiglib + - name: net-tools + clone_depth: 1 + west-commands: scripts/west-commands.yml + self: + path: zephyr + ''' + kconfiglib, net_tools, zephyr = [os.path.join(_session_repos, x) for x in + ['Kconfiglib', 'net-tools', 'zephyr']] + repos = tmpdir.mkdir('repos') + repos.chdir() + for r in [kconfiglib, net_tools, zephyr]: + subprocess.check_call([GIT, 'clone', r]) + + manifest = MANIFEST_TEMPLATE.replace('THE_URL_BASE', + str(tmpdir.join('repos'))) + add_commit(str(repos.join('zephyr')), 'add manifest', + files={'west.yml': manifest}) + return tmpdir @pytest.fixture def west_init_tmpdir(repos_tmpdir): @@ -193,8 +207,10 @@ def cmd(cmd, cwd=None, stderr=None): # a python subprocess so that program-level setup and teardown # happen fresh. + cmdlst = shlex.split('west ' + cmd) + print('running:', cmdlst) try: - return check_output(shlex.split('west ' + cmd), cwd=cwd, stderr=stderr) + return check_output(cmdlst, cwd=cwd, stderr=stderr) except subprocess.CalledProcessError: print('cmd: west:', shutil.which('west'), file=sys.stderr) raise From 6dd7b53f58921fd6c23a12d6754c31ed3b3ca0bc Mon Sep 17 00:00:00 2001 From: Marti Bolivar Date: Thu, 2 May 2019 08:18:13 -0600 Subject: [PATCH 05/15] main: fix CalledProcessError handling I'm not sure this ever worked; the args field is an empty tuple even in Python 3.4. Use cmd and returncode attributes appropriately instead. Don't offer the 'for a stack trace' message here anymore: this doesn't indicate a west error, which is what that is meant to capture. Signed-off-by: Marti Bolivar --- src/west/main.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/west/main.py b/src/west/main.py index bb858bd2..26a414b6 100755 --- a/src/west/main.py +++ b/src/west/main.py @@ -557,12 +557,10 @@ def main(argv=None): except KeyboardInterrupt: sys.exit(0) except CalledProcessError as cpe: - log.err('command exited with status {}: {}'.format( - cpe.args[0], quote_sh_list(cpe.args[1]))) + log.err('command exited with status {}: {}'. + format(cpe.returncode, quote_sh_list(cpe.cmd))) if args.verbose: traceback.print_exc() - else: - log.inf(for_stack_trace) sys.exit(cpe.returncode) except ExtensionCommandError as ece: log.err('extension command', args.command, From 18408dd9acf0df5b99d666b447bcb2cdd1794705 Mon Sep 17 00:00:00 2001 From: Marti Bolivar Date: Thu, 2 May 2019 08:36:48 -0600 Subject: [PATCH 06/15] main.py: don't throw away tracebacks Add a helper for saving the current traceback to a temporary file and use it appropriately from the exception handlers in main(), This avoid throwing away information in case the error was nondeterministic. Signed-off-by: Marti Bolivar --- src/west/main.py | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/west/main.py b/src/west/main.py index 26a414b6..66c4641c 100755 --- a/src/west/main.py +++ b/src/west/main.py @@ -19,6 +19,7 @@ import shutil import sys from subprocess import CalledProcessError +import tempfile import textwrap import traceback @@ -522,6 +523,15 @@ def get_extension_commands(): return extensions +def dump_traceback(): + # Save the current exception to a file and return its path. + fd, name = tempfile.mkstemp(prefix='west-exc-', suffix='.txt') + os.close(fd) # traceback has no use for the fd + with open(name, 'w') as f: + traceback.print_exc(file=f) + return name + + def main(argv=None): # Makes ANSI color escapes work on Windows, and strips them when # stdout/stderr isn't a terminal @@ -550,8 +560,6 @@ def main(argv=None): argv = sys.argv[1:] args, unknown = parse_args(argv, extensions, topdir) - for_stack_trace = 'run as "west -v {}" for a stack trace'.format( - quote_sh_list(argv)) try: args.handler(args, unknown) except KeyboardInterrupt: @@ -563,19 +571,22 @@ def main(argv=None): traceback.print_exc() sys.exit(cpe.returncode) except ExtensionCommandError as ece: - log.err('extension command', args.command, - 'was improperly defined and could not be run{}'. - format(': ' + ece.hint if ece.hint else '')) + msg = 'extension command "{}" could not be run{}.'.format( + args.command, ': ' + ece.hint if ece.hint else '') if args.verbose: + log.err(msg) traceback.print_exc() else: - log.inf(for_stack_trace) + log.err(msg, 'See {} for a traceback.'.format(dump_traceback())) sys.exit(ece.returncode) except CommandContextError as cce: log.err('command', args.command, 'cannot be run in this context:', *cce.args) + log.err('see {} for a traceback.'.format(dump_traceback())) sys.exit(cce.returncode) except CommandError as ce: + # No need to dump_traceback() here. The command is responsible + # for logging its own errors. sys.exit(ce.returncode) From f98b719a4eb4280a661baccfa850f2613d709eba Mon Sep 17 00:00:00 2001 From: Marti Bolivar Date: Tue, 14 May 2019 14:12:02 -0600 Subject: [PATCH 07/15] manifest: demote Project remote to a kwarg We're going to add explicit project URLs to the manifest, so the remote should no longer be a positional. Signed-off-by: Marti Bolivar --- src/west/manifest.py | 18 +++++++++--------- tests/test_manifest.py | 24 ++++++++++++------------ 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/west/manifest.py b/src/west/manifest.py index 42c26bd2..ccd3f93d 100644 --- a/src/west/manifest.py +++ b/src/west/manifest.py @@ -265,12 +265,12 @@ def _load(self, data): # Create the project instance for final checking. project = Project(name, - remotes_dict[remote_name], defaults, path=mp.get('path'), clone_depth=mp.get('clone-depth'), revision=mp.get('revision'), - west_commands=mp.get('west-commands')) + west_commands=mp.get('west-commands'), + remote=remotes_dict[remote_name]) # Two projects cannot have the same path. We use absolute # paths to check for collisions to ensure paths are @@ -389,14 +389,11 @@ class Project: __slots__ = ('name remote url path abspath posixpath clone_depth ' 'revision west_commands').split() - def __init__(self, name, remote, defaults, path=None, clone_depth=None, - revision=None, west_commands=None): + def __init__(self, name, defaults, path=None, clone_depth=None, + revision=None, west_commands=None, remote=None): '''Specify a Project by name, Remote, and optional information. :param name: Project's user-defined name in the manifest. - :param remote: Remote instance corresponding to this Project as - specified in the manifest. This is used to build - the project's URL, and is also stored as an attribute. :param defaults: If the revision parameter is not given, the project's revision is set to defaults.revision. :param path: Relative path to the project in the west @@ -409,13 +406,14 @@ def __init__(self, name, remote, defaults, path=None, clone_depth=None, :param west_commands: path to a YAML file in the project containing a description of west extension commands provided by the project, if given. + :param remote: Remote instance corresponding to this Project as + specified in the manifest. This is used to build + the project's URL, and is also stored as an attribute. ''' _wrn_if_not_remote(remote) self.name = name '''Project name as it appears in the manifest.''' - self.remote = remote - '''`Remote` instance corresponding to the project's remote.''' self.url = remote.url_base + '/' + name '''Complete fetch URL for the project.''' self.path = os.path.normpath(path or name) @@ -432,6 +430,8 @@ def __init__(self, name, remote, defaults, path=None, clone_depth=None, from manifest defaults, or from the default supplied by west.''' self.west_commands = west_commands '''Path to project's "west-commands", or None.''' + self.remote = remote + '''`Remote` instance corresponding to the project's remote.''' def __eq__(self, other): return NotImplemented diff --git a/tests/test_manifest.py b/tests/test_manifest.py index 3fe44d38..b2103f93 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -78,10 +78,10 @@ def test_no_defaults(config_file_project_setup): manifest = Manifest.from_data(yaml.safe_load(content)) expected = [ManifestProject(path='manifestproject'), - Project('testproject1', r1, None, path='testproject1', - clone_depth=None, revision='rev1'), - Project('testproject2', r2, None, path='testproject2', - clone_depth=None, revision='master')] + Project('testproject1', None, path='testproject1', + clone_depth=None, revision='rev1', remote=r1), + Project('testproject2', None, path='testproject2', + clone_depth=None, revision='master', remote=r2)] # Check the remotes are as expected. assert list(manifest.remotes) == [r1, r2] @@ -119,10 +119,10 @@ def test_self_tag(project_setup): manifest = Manifest.from_data(yaml.safe_load(content)) expected = [ManifestProject(path='mainproject'), - Project('testproject1', r1, None, path='testproject1', - clone_depth=None, revision='rev1'), - Project('testproject2', r2, None, path='testproject2', - clone_depth=None, revision='master')] + Project('testproject1', None, path='testproject1', + clone_depth=None, revision='rev1', remote=r1), + Project('testproject2', None, path='testproject2', + clone_depth=None, revision='master', remote=r2)] # Check the remotes are as expected. assert list(manifest.remotes) == [r1, r2] @@ -162,10 +162,10 @@ def test_default_clone_depth(config_file_project_setup): manifest = Manifest.from_data(yaml.safe_load(content)) expected = [ManifestProject(path='manifestproject'), - Project('testproject1', r1, d, path='testproject1', - clone_depth=None, revision=d.revision), - Project('testproject2', r2, d, path='testproject2', - clone_depth=1, revision='rev')] + Project('testproject1', d, path='testproject1', + clone_depth=None, revision=d.revision, remote=r1), + Project('testproject2', d, path='testproject2', + clone_depth=1, revision='rev', remote=r2)] # Check that default attributes match. assert manifest.defaults.remote == d.remote From f87254a0478a1085e3f4ebe6da54a44c4837949c Mon Sep 17 00:00:00 2001 From: Marti Bolivar Date: Tue, 14 May 2019 14:39:36 -0600 Subject: [PATCH 08/15] manifest: support explicit project URLs Allow each project element to explicitly specify its URL. This avoids forcing users to name projects according to their URLs, which can be inconvenient (and prevents us from enforcing a rule that project names are unique). Signed-off-by: Marti Bolivar --- src/west/manifest-schema.yml | 6 +++- src/west/manifest.py | 63 +++++++++++++++++++++--------------- 2 files changed, 42 insertions(+), 27 deletions(-) diff --git a/src/west/manifest-schema.yml b/src/west/manifest-schema.yml index 57cd5203..1484832d 100644 --- a/src/west/manifest-schema.yml +++ b/src/west/manifest-schema.yml @@ -36,7 +36,7 @@ mapping: # and a fetch URL base. These URL bases are concatenated with project names # (separated by a /) to form complete fetch URLs. remotes: - required: true + required: false type: seq sequence: - type: map @@ -64,6 +64,10 @@ mapping: remote: required: false type: str + # Explicit project URL (may not be given with a remote as well) + url: + required: false + type: str # Revision to check out. May be a branch, tag, or SHA. revision: required: false diff --git a/src/west/manifest.py b/src/west/manifest.py index ccd3f93d..84c2e407 100644 --- a/src/west/manifest.py +++ b/src/west/manifest.py @@ -144,7 +144,7 @@ def __init__(self, source_file=None, source_data=None): self.remotes = None '''Sequence of west.manifest.Remote objects representing manifest - remotes.''' + remotes. Note that not all projects have a remote.''' self.projects = None '''Sequence of west.manifest.Project objects representing manifest @@ -226,7 +226,7 @@ def _load(self, data): # Map from each remote's name onto that remote's data in the manifest. remotes = tuple(Remote(r['name'], r['url-base']) for r in - manifest['remotes']) + manifest.get('remotes', [])) remotes_dict = {r.name: r for r in remotes} # Get any defaults out of the manifest. @@ -254,23 +254,32 @@ def _load(self, data): # Validate the project name. name = mp['name'] - # Validate the project remote. + # Validate the project remote or URL. remote_name = mp.get('remote', default_remote_name) - if remote_name is None: - self._malformed('project {} does not specify a remote'. + url = mp.get('url') + if remote_name is None and url is None: + self._malformed('project {} does not specify a remote or URL'. format(name)) - if remote_name not in remotes_dict: - self._malformed('project {} remote {} is not defined'. - format(name, remote_name)) + if remote_name: + if remote_name not in remotes_dict: + self._malformed('project {} remote {} is not defined'. + format(name, remote_name)) + remote = remotes_dict[remote_name] + else: + remote = None # Create the project instance for final checking. - project = Project(name, - defaults, - path=mp.get('path'), - clone_depth=mp.get('clone-depth'), - revision=mp.get('revision'), - west_commands=mp.get('west-commands'), - remote=remotes_dict[remote_name]) + try: + project = Project(name, + defaults, + path=mp.get('path'), + clone_depth=mp.get('clone-depth'), + revision=mp.get('revision'), + west_commands=mp.get('west-commands'), + remote=remote, + url=url) + except ValueError as ve: + self._malformed(ve.args[0]) # Two projects cannot have the same path. We use absolute # paths to check for collisions to ensure paths are @@ -317,8 +326,8 @@ def __init__(self, remote=None, revision=None): or None (an actual Remote object, not the name of a remote as a string). :param revision: Default Git revision; 'master' if not given.''' - if remote is not None: - _wrn_if_not_remote(remote) + if remote is not None and not isinstance(remote, Remote): + raise ValueError('{} is not a Remote'.format(remote)) if revision is None: revision = 'master' @@ -390,7 +399,7 @@ class Project: 'revision west_commands').split() def __init__(self, name, defaults, path=None, clone_depth=None, - revision=None, west_commands=None, remote=None): + revision=None, west_commands=None, remote=None, url=None): '''Specify a Project by name, Remote, and optional information. :param name: Project's user-defined name in the manifest. @@ -409,13 +418,19 @@ def __init__(self, name, defaults, path=None, clone_depth=None, :param remote: Remote instance corresponding to this Project as specified in the manifest. This is used to build the project's URL, and is also stored as an attribute. + :param url: The project's fetch URL. This cannot be given with `remote` + as well: choose one. ''' - _wrn_if_not_remote(remote) + if remote and url: + raise ValueError('got remote={} and url={}'.format(remote, url)) + if not (remote or url): + raise ValueError('got neither a remote nor a URL') self.name = name '''Project name as it appears in the manifest.''' - self.url = remote.url_base + '/' + name - '''Complete fetch URL for the project.''' + self.url = url or (remote.url_base + '/' + name) + '''Complete fetch URL for the project, either as given by the url kwarg + or computed from the remote URL base and the project name.''' self.path = os.path.normpath(path or name) '''Relative path to the project in the installation.''' self.abspath = os.path.realpath(os.path.join(util.west_topdir(), @@ -431,7 +446,7 @@ def __init__(self, name, defaults, path=None, clone_depth=None, self.west_commands = west_commands '''Path to project's "west-commands", or None.''' self.remote = remote - '''`Remote` instance corresponding to the project's remote.''' + '''`Remote` instance corresponding to the project's remote, or None.''' def __eq__(self, other): return NotImplemented @@ -650,9 +665,5 @@ def as_dict(self): ret['west-commands'] = self.west_commands return ret -def _wrn_if_not_remote(remote): - if not isinstance(remote, Remote): - log.wrn('Remote', remote, 'is not a Remote instance') - _SCHEMA_PATH = os.path.join(os.path.dirname(__file__), "manifest-schema.yml") From 58ee6de21f4bcd60786d3922dc2e6895c319ba7f Mon Sep 17 00:00:00 2001 From: Marti Bolivar Date: Tue, 14 May 2019 14:41:37 -0600 Subject: [PATCH 09/15] manifest: demote defaults to a kwarg We don't apply the west default revision correctly if defaults is None. Since we find it convenient to let west figure out the defaults for us, demote it to a kwarg as well and let it be None more gracefully. Signed-off-by: Marti Bolivar --- src/west/manifest.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/west/manifest.py b/src/west/manifest.py index 84c2e407..4997ea5c 100644 --- a/src/west/manifest.py +++ b/src/west/manifest.py @@ -398,13 +398,14 @@ class Project: __slots__ = ('name remote url path abspath posixpath clone_depth ' 'revision west_commands').split() - def __init__(self, name, defaults, path=None, clone_depth=None, + def __init__(self, name, defaults=None, path=None, clone_depth=None, revision=None, west_commands=None, remote=None, url=None): '''Specify a Project by name, Remote, and optional information. :param name: Project's user-defined name in the manifest. :param defaults: If the revision parameter is not given, the project's - revision is set to defaults.revision. + revision is set to defaults.revision if defaults is + not None, or the west-wide default otherwise. :param path: Relative path to the project in the west installation, if present in the manifest. If not given, the project's ``name`` is used. @@ -426,6 +427,9 @@ def __init__(self, name, defaults, path=None, clone_depth=None, if not (remote or url): raise ValueError('got neither a remote nor a URL') + if defaults is None: + defaults = _DEFAULTS + self.name = name '''Project name as it appears in the manifest.''' self.url = url or (remote.url_base + '/' + name) @@ -667,3 +671,4 @@ def as_dict(self): _SCHEMA_PATH = os.path.join(os.path.dirname(__file__), "manifest-schema.yml") +_DEFAULTS = Defaults() From a77438028bc21c443990bf662917b595436b6cd3 Mon Sep 17 00:00:00 2001 From: Marti Bolivar Date: Tue, 14 May 2019 14:43:35 -0600 Subject: [PATCH 10/15] tests: add test cases for explicit project URLs Make sure to update comments for existing tests with modified semantics. Signed-off-by: Marti Bolivar --- .../manifests/invalid_ill_defined_project.yml | 2 +- tests/manifests/invalid_no_remotes.yml | 2 + tests/manifests/invalid_remote_and_url.yml | 9 ++++ tests/test_manifest.py | 51 +++++++++++++++++++ 4 files changed, 63 insertions(+), 1 deletion(-) create mode 100644 tests/manifests/invalid_remote_and_url.yml diff --git a/tests/manifests/invalid_ill_defined_project.yml b/tests/manifests/invalid_ill_defined_project.yml index b7355426..e0cd7e5d 100644 --- a/tests/manifests/invalid_ill_defined_project.yml +++ b/tests/manifests/invalid_ill_defined_project.yml @@ -4,4 +4,4 @@ manifest: url-base: https://example.com projects: - - name: ill_defined_no_remote + - name: ill_defined_no_remote_or_url diff --git a/tests/manifests/invalid_no_remotes.yml b/tests/manifests/invalid_no_remotes.yml index a761731f..c2d879e2 100644 --- a/tests/manifests/invalid_no_remotes.yml +++ b/tests/manifests/invalid_no_remotes.yml @@ -1,3 +1,5 @@ +# If any project does not have a URL specified, there must be a remote +# section and a default remote set to it. manifest: projects: - name: testproject diff --git a/tests/manifests/invalid_remote_and_url.yml b/tests/manifests/invalid_remote_and_url.yml new file mode 100644 index 00000000..62321640 --- /dev/null +++ b/tests/manifests/invalid_remote_and_url.yml @@ -0,0 +1,9 @@ +# No project element may explicitly specify both a remote and a URL. +manifest: + remotes: + - name: remote1 + url-base: https://example.com + projects: + - name: project1 + remote: remote1 + url: https://example.com/explicit-project-url diff --git a/tests/test_manifest.py b/tests/test_manifest.py index b2103f93..244d989c 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -4,6 +4,7 @@ from glob import glob import os +import platform from unittest.mock import patch import pytest @@ -53,6 +54,56 @@ def deep_eq_check(actual, expected): assert actual.revision == expected.revision assert actual.west_commands == expected.west_commands +@patch('west.util.west_topdir', return_value='/west_top') +def test_init_with_url(west_topdir): + # Test the project constructor works as expected with a URL. + + p = Project('p', url='some-url') + assert p.name == 'p' + assert p.remote is None + assert p.url == 'some-url' + assert p.path == 'p' + assert p.abspath == os.path.realpath(os.path.join('/west_top', 'p')) + posixpath = p.posixpath + if platform.system() == 'Windows': + posixpath = os.path.splitdrive(posixpath)[1] + assert posixpath == '/west_top/p' + assert p.clone_depth is None + assert p.revision == 'master' + assert p.west_commands is None + +@patch('west.util.west_topdir', return_value='/west_top') +def test_remote_url_init(west_topdir): + # Projects must be initialized with a remote or a URL, but not both. + # The resulting URLs must behave as documented. + + r1 = Remote('testremote1', 'https://example.com') + p1 = Project('project1', None, remote=r1) + assert p1.remote is r1 + assert p1.url == 'https://example.com/project1' + + p2 = Project('project2', None, url='https://example.com/project2') + assert p2.remote is None + assert p2.url == 'https://example.com/project2' + + with pytest.raises(ValueError): + Project('project3', None, remote=r1, url='not-empty') + + with pytest.raises(ValueError): + Project('project4', None, remote=None, url=None) + +@patch('west.util.west_topdir', return_value='/west_top') +def test_no_remote_ok(west_topdir): + # remotes isn't required if projects are specified by URL. + + content = '''\ + manifest: + projects: + - name: testproject + url: https://example.com/my-project + ''' + manifest = Manifest.from_data(yaml.safe_load(content)) + assert manifest.projects[1].url == 'https://example.com/my-project' def test_no_defaults(config_file_project_setup): # Manifests with no defaults should work. From 0d079744de106bfb35994988b1f1ab164180901a Mon Sep 17 00:00:00 2001 From: Marti Bolivar Date: Tue, 14 May 2019 15:05:45 -0600 Subject: [PATCH 11/15] manifest: project names must be unique It is always possible to satisfy this constraint now that project URLs may be specified explicitly. (This restriction will be necessary to make manifest imports work sensibly -- we should have done this before...). Signed-off-by: Marti Bolivar --- src/west/manifest.py | 6 ++++++ tests/manifests/invalid_duplicate_name.yml | 1 + 2 files changed, 7 insertions(+) create mode 100644 tests/manifests/invalid_duplicate_name.yml diff --git a/src/west/manifest.py b/src/west/manifest.py index 4997ea5c..8d7fb042 100644 --- a/src/west/manifest.py +++ b/src/west/manifest.py @@ -210,6 +210,7 @@ def _load(self, data): # Initialize this instance's fields from values given in the # manifest data, which must be validated according to the schema. projects = [] + project_names = set() project_abspaths = set() manifest = data.get('manifest') @@ -281,6 +282,10 @@ def _load(self, data): except ValueError as ve: self._malformed(ve.args[0]) + # Project names must be unique. + if project.name in project_names: + self._malformed('project name {} is already used'. + format(project.name)) # Two projects cannot have the same path. We use absolute # paths to check for collisions to ensure paths are # normalized (e.g. for case-insensitive file systems or @@ -290,6 +295,7 @@ def _load(self, data): self._malformed('project {} path {} is already in use'. format(project.name, project.path)) + project_names.add(project.name) project_abspaths.add(project.abspath) projects.append(project) diff --git a/tests/manifests/invalid_duplicate_name.yml b/tests/manifests/invalid_duplicate_name.yml new file mode 100644 index 00000000..8b137891 --- /dev/null +++ b/tests/manifests/invalid_duplicate_name.yml @@ -0,0 +1 @@ + From ebae08f601c9f631dadfc39df7ac94030996c865 Mon Sep 17 00:00:00 2001 From: Marti Date: Wed, 15 May 2019 11:29:27 -0600 Subject: [PATCH 12/15] util: add canon_path() We need a common definition for canonicalizing paths; the lack of one is causing issues on Windows. Signed-off-by: Marti Bolivar --- src/west/util.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/west/util.py b/src/west/util.py index 34b758ce..26fbdba1 100644 --- a/src/west/util.py +++ b/src/west/util.py @@ -42,6 +42,17 @@ import yaml +def canon_path(path): + '''Returns a canonical version of the path. + + This is currently ``os.path.normcase(os.path.abspath(path))``. The + path separator is converted to os.sep on platforms where that + matters (Windows). + + :param path: path whose canonical name to return; need not + refer to an existing file. + ''' + return os.path.normcase(os.path.abspath(path)) def escapes_directory(path, directory): '''Returns True if `path` escapes parent directory `directory`. From 0fcdc25d4699fd64dbcfb4e878c44d6ab9090609 Mon Sep 17 00:00:00 2001 From: Marti Date: Wed, 15 May 2019 11:57:41 -0600 Subject: [PATCH 13/15] tests: fix test_config.py on windows 'west config' testing is giving false negatives on Windows. To fix: - canonicalize and fix up global config path testing - avoid passing paths to cmd(); they don't play well with shlex.split() and aren't necessary to make sure the config command is behaving properly Signed-off-by: Marti Bolivar --- tests/test_config.py | 95 +++++++++++++++++++++++--------------------- 1 file changed, 49 insertions(+), 46 deletions(-) diff --git a/tests/test_config.py b/tests/test_config.py index 6a3e558f..a9f1f088 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -16,6 +16,7 @@ import pytest import subprocess from west import configuration as config +from west.util import canon_path from conftest import cmd @@ -31,72 +32,78 @@ def test_config_global(west_init_tmpdir): # To ensure that GLOBAL home folder points into tox temp dir. # Otherwise fail the test, as we don't want to risk manipulating user's # west config - assert config.ConfigFile.GLOBAL.value == \ - os.path.join(os.environ.get('TOXTEMPDIR'), 'pytest-home/.westconfig') + assert canon_path(config.ConfigFile.GLOBAL.value) == \ + canon_path(os.path.join(os.environ.get('TOXTEMPDIR'), + 'pytest-home', '.westconfig')) + # Make sure the value is currently unset. testkey_value = cmd('config pytest.testkey_global') assert testkey_value == '' - # Set value in user's testing home - cmd('config --global pytest.testkey_global ' + str(west_init_tmpdir)) + # Set value globally. + cmd('config --global pytest.testkey_global foo') - # Readback from --local, to ensure that is empty. + # Read from --local, to ensure that is empty. testkey_value = cmd('config --local pytest.testkey_global') assert testkey_value == '' - # Readback from --system, to ensure that is empty. + # Read from --system, to ensure that is empty. testkey_value = cmd('config --system pytest.testkey_global') assert testkey_value == '' - # Readback from --global, and compare the value with the expected. + # Read from --global, and check the value. testkey_value = cmd('config --global pytest.testkey_global') - assert testkey_value.strip('\n') == str(west_init_tmpdir) + assert testkey_value.rstrip() == 'foo' - # Readback in from all files should also provide the value. + # Without an explicit config source, the global value (the only + # one set) should be returned. testkey_value = cmd('config pytest.testkey_global') - assert testkey_value.strip('\n') == str(west_init_tmpdir) + assert testkey_value.rstrip() == 'foo' def test_config_local(west_init_tmpdir): if not os.path.exists(os.path.expanduser('~')): os.mkdir(os.path.expanduser('~')) - test_value = str(west_init_tmpdir) + '_local' - testkey_value = cmd('config pytest.testkey_local') assert testkey_value == '' - # Set value in project local - cmd('config --local pytest.testkey_local ' + test_value) + # Set a config option in the installation. + cmd('config --local pytest.testkey_local foo') - # Readback from --global, to ensure that is empty. + # It should not be available in the global or system files. testkey_value = cmd('config --global pytest.testkey_local') assert testkey_value == '' + testkey_value = cmd('config --system pytest.testkey_local') + assert testkey_value == '' - # Readback from --local, and compare the value with the expected. + # It should be available with --local. testkey_value = cmd('config --local pytest.testkey_local') - assert testkey_value.strip('\n') == test_value + assert testkey_value.rstrip() == 'foo' - # Readback in from all files should also provide the value. + # Without an explicit config source, the local value (the only one + # set) should be returned. testkey_value = cmd('config pytest.testkey_local') - assert testkey_value.strip('\n') == test_value + assert testkey_value.rstrip() == 'foo' - # Update the value in user's testing home without --local and see it's - # default to --local when reading back - test_value_update = str(west_init_tmpdir) + '_update' - cmd('config pytest.testkey_local ' + test_value_update) + # Update the value without a config destination, which should + # default to --local. + cmd('config pytest.testkey_local foo2') - # Readback from --global, to ensure that is empty. + # The --global and --system settings should still be empty. testkey_value = cmd('config --global pytest.testkey_local') assert testkey_value == '' + testkey_value = cmd('config --system pytest.testkey_local') + assert testkey_value == '' - # Readback from --local, and compare the value with the expected. + # Read from --local, and check the value. testkey_value = cmd('config --local pytest.testkey_local') - assert testkey_value.strip('\n') == test_value_update + assert testkey_value.rstrip() == 'foo2' - # Readback in from all files should also provide the value. + # Without an explicit config source, the local value (the only one + # set) should be returned. testkey_value = cmd('config pytest.testkey_local') - assert testkey_value.strip('\n') == test_value_update + assert testkey_value.rstrip() == 'foo2' # We skip this test if executed directly using pytest, to avoid modifying @@ -108,36 +115,32 @@ def test_config_precendence(west_init_tmpdir): if not os.path.exists(os.path.expanduser('~')): os.mkdir(os.path.expanduser('~')) - test_value_local = str(west_init_tmpdir) + '_precedence' - test_value_global = str(west_init_tmpdir) + '_global' - + # Make sure the value is not set. testkey_value = cmd('config pytest.testkey_precedence') assert testkey_value == '' - # Set value in user's testing home - cmd('config --global pytest.testkey_precedence ' + test_value_global) - - # Readback from --global, to verify it is set. + # Set value globally and verify it is set. + cmd('config --global pytest.testkey_precedence foo_global') testkey_value = cmd('config --global pytest.testkey_precedence') - assert testkey_value.strip('\n') == test_value_global + assert testkey_value.rstrip() == 'foo_global' - # Readback from --local, to ensure it is not available using --local + # Read with --local and ensure it is not set. testkey_value = cmd('config --local pytest.testkey_precedence') - assert testkey_value.strip('\n') == '' + assert testkey_value.rstrip() == '' - # Set value in project local and verify it can be read back. - cmd('config --local pytest.testkey_precedence ' + test_value_local) + # Set with --local and verify it is set. + cmd('config --local pytest.testkey_precedence foo_local') testkey_value = cmd('config --local pytest.testkey_precedence') - assert testkey_value.strip('\n') == test_value_local + assert testkey_value.rstrip() == 'foo_local' - # Readback without specifying --local or --global and see that project - # specific value takes precedence. + # Read without specifying --local or --global and verify that + # --local takes precedence. testkey_value = cmd('config pytest.testkey_precedence') - assert testkey_value.strip('\n') == test_value_local + assert testkey_value.rstrip() == 'foo_local' - # Make additional verification that --global is still available. + # Make sure the --global value is still available. testkey_value = cmd('config --global pytest.testkey_precedence') - assert testkey_value.strip('\n') == test_value_global + assert testkey_value.rstrip() == 'foo_global' def test_config_missing_key(west_init_tmpdir): From 781ac4640ee47822a0ea7bcf605dfb272498086e Mon Sep 17 00:00:00 2001 From: Marti Date: Wed, 15 May 2019 12:18:54 -0600 Subject: [PATCH 14/15] tests: conftest: use command strings exactly on Windows This fixes test_project.py::test_list on Windows, as otherwise quoting and splitting the relevant paths using shlex with POSIX rules behaves incorrectly. Signed-off-by: Marti Bolivar --- tests/conftest.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index b1d963d9..95f519df 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,4 +1,5 @@ import os +import platform import shlex import shutil import subprocess @@ -195,9 +196,7 @@ def check_output(*args, **kwargs): def cmd(cmd, cwd=None, stderr=None): # Run a west command in a directory (cwd defaults to os.getcwd()). # - # This helper takes the command as a string, which is less clunky - # to work with than a list. It is split according to shell rules - # before being run. + # This helper takes the command as a string. # # This helper relies on the test environment to ensure that the # 'west' executable is a bootstrapper installed from the current @@ -206,11 +205,12 @@ def cmd(cmd, cwd=None, stderr=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. - - cmdlst = shlex.split('west ' + cmd) - print('running:', cmdlst) + cmd = 'west ' + cmd + if platform.system() != 'Windows': + cmd = shlex.split(cmd) + print('running:', cmd) try: - return check_output(cmdlst, cwd=cwd, stderr=stderr) + return check_output(cmd, cwd=cwd, stderr=stderr) except subprocess.CalledProcessError: print('cmd: west:', shutil.which('west'), file=sys.stderr) raise From 2df535a4ca9fffa068681912c52771088849c3d6 Mon Sep 17 00:00:00 2001 From: Marti Date: Wed, 15 May 2019 12:23:42 -0600 Subject: [PATCH 15/15] tests: fix test_project.py on Windows This file is giving false negatives on Windows. To fix: - test_manifest_freeze: expect (and escape) Windows paths - test_forall: echo may be available in cmd.exe, but single-quoted strings sure aren't - test_extension_command_*: handle newlines portably Signed-off-by: Marti Bolivar --- tests/test_project.py | 46 ++++++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 22 deletions(-) diff --git a/tests/test_project.py b/tests/test_project.py index b3302a5e..4a4babf4 100644 --- a/tests/test_project.py +++ b/tests/test_project.py @@ -90,6 +90,7 @@ def test_manifest_freeze(west_update_tmpdir): # match project order as specified in our manifest, that all # revisions are full 40-character SHAs, and there isn't any random # YAML tag crap. + kconfig_rel = os.path.join('subdir', 'Kconfiglib') expected_res = ['^manifest:$', '^ defaults:$', '^ remote: test-local$', @@ -101,7 +102,7 @@ def test_manifest_freeze(west_update_tmpdir): '^ - name: Kconfiglib$', '^ remote: test-local$', '^ revision: [a-f0-9]{40}$', - '^ path: subdir/Kconfiglib$', + '^ path: {}$'.format(re.escape(kconfig_rel)), '^ - name: net-tools$', '^ remote: test-local$', '^ revision: [a-f0-9]{40}$', @@ -151,15 +152,15 @@ def test_forall(west_init_tmpdir): # 'forall' with no projects cloned shouldn't fail - cmd("forall -c 'echo *'") + cmd('forall -c "echo *"') # Neither should it fail after cloning one or both projects cmd('update net-tools') - cmd("forall -c 'echo *'") + cmd('forall -c "echo *"') cmd('update Kconfiglib') - cmd("forall -c 'echo *'") + cmd('forall -c "echo *"') def test_update_projects(west_init_tmpdir): @@ -294,7 +295,7 @@ def test_extension_command_execution(west_init_tmpdir): cmd('update') actual = cmd('test') - assert actual == 'Testing test command 1\n' + assert actual.rstrip() == 'Testing test command 1' def test_extension_command_multiproject(repos_tmpdir): @@ -360,22 +361,22 @@ def do_run(self, args, ignored): config.read_config() cmd('update') - help_text = cmd('-h') - expected = textwrap.dedent('''\ - commands from project at "subdir/Kconfiglib": - kconfigtest: (no help provided; try "west kconfigtest -h") - - commands from project at "net-tools": - test: test-help - ''') - - assert expected in help_text + # The newline shenanigans are for Windows. + help_text = '\n'.join(cmd('-h').splitlines()) + expected = '\n'.join([ + 'commands from project at "{}":'.format(os.path.join('subdir', + 'Kconfiglib')), + ' kconfigtest: (no help provided; try "west kconfigtest -h")', # noqa: E501 + '', + 'commands from project at "net-tools":', + ' test: test-help']) + assert expected in help_text, help_text actual = cmd('test') - assert actual == 'Testing test command 1\n' + assert actual.rstrip() == 'Testing test command 1' actual = cmd('kconfigtest') - assert actual == 'Testing kconfig test\n' + assert actual.rstrip() == 'Testing kconfig test' def test_extension_command_duplicate(repos_tmpdir): @@ -440,12 +441,13 @@ def do_run(self, args, ignored): config.read_config() cmd('update') - actual = cmd('test', stderr=subprocess.STDOUT) - warning = 'WARNING: ignoring project net-tools extension command "test";'\ - ' command "test" already defined as extension command\n' - command_out = 'Testing kconfig test command\n' + actual = cmd('test', stderr=subprocess.STDOUT).splitlines() + expected = [ + 'WARNING: ignoring project net-tools extension command "test"; command "test" already defined as extension command', # noqa: E501 + 'Testing kconfig test command', + ] - assert actual == warning + command_out + assert actual == expected #