Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Multimanifest prep #266

Merged
merged 15 commits into from
May 20, 2019
Merged

Multimanifest prep #266

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
41fbf25
tests: flatten the hierarchy
mbolivar-nordic May 2, 2019
8d329be
tests: remove empty_config.py
mbolivar-nordic May 14, 2019
884170e
tests: don't test pre-unification features
mbolivar-nordic May 14, 2019
1cb9be1
tests: fixture setup optimizations
mbolivar-nordic May 14, 2019
6dd7b53
main: fix CalledProcessError handling
mbolivar-nordic May 2, 2019
18408dd
main.py: don't throw away tracebacks
mbolivar-nordic May 2, 2019
f98b719
manifest: demote Project remote to a kwarg
mbolivar-nordic May 14, 2019
f87254a
manifest: support explicit project URLs
mbolivar-nordic May 14, 2019
58ee6de
manifest: demote defaults to a kwarg
mbolivar-nordic May 14, 2019
a774380
tests: add test cases for explicit project URLs
mbolivar-nordic May 14, 2019
0d07974
manifest: project names must be unique
mbolivar-nordic May 14, 2019
ebae08f
util: add canon_path()
mbolivar-nordic May 15, 2019
0fcdc25
tests: fix test_config.py on windows
mbolivar-nordic May 15, 2019
781ac46
tests: conftest: use command strings exactly on Windows
mbolivar-nordic May 15, 2019
2df535a
tests: fix test_project.py on Windows
mbolivar-nordic May 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import os
import platform
import shlex
import shutil
import subprocess
Expand Down Expand Up @@ -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
Expand All @@ -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':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We typically use sys.platform, maybe use that for consistency

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed btw? We don't usually make this difference in normal invokations of check_output et al.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We typically use sys.platform, maybe use that for consistency

Really? We use platform.system() in configuration.py ,which is what this is testing. I can make that change, though.

Why is this needed btw? We don't usually make this difference in normal invokations of check_output et al.

It's breaking the tests because shlex splits by POSIX rules, which isn't right for windows shells and led to bad results when applied to paths. Example:

In [2]: shlex.split('west foo bar\\baz')
Out[2]: ['west', 'foo', 'barbaz']

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I see. I don't have a strong preference either way, just wanted to keep it consistent. I didn't know we used platform.system() in configuration.py, but I've always seen sys.platform elsewhere, like the tox.ini or the requirements.txt.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 years later, dropping shlex.split() in #730

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
Expand Down