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.

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

Support for running setup.py nosetests #7

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
46 changes: 33 additions & 13 deletions nosepipe.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

import nose.plugins

__version__ = "0.5"
__version__ = "0.5-post3"
Copy link
Owner

Choose a reason for hiding this comment

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

You can leave out any version changes, as those will be done when a new release is made - right now this might conflict as 0.6 has been released.


SUBPROCESS_ENV_KEY = "NOSE_WITH_PROCESS_ISOLATION_REPORTER"

Expand Down Expand Up @@ -147,12 +147,16 @@ def __call__(self, result):
useshell = True

self.logger.debug("Executing %s", " ".join(argv))
popen = subprocess.Popen(argv,
cwd=self._cwd,
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
shell=useshell,
)
try:
popen = subprocess.Popen(argv,
cwd=self._cwd,
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
shell=useshell,
)
except OSError, e:
Copy link
Owner

Choose a reason for hiding this comment

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

Just tried this with Python 3 and noticed this is Python 2 only code - it should instead be except OSError as e:.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this as well as the # in comment. How are you testing? I’ve tried setup.py nosetests and get lots of problems.

On Feb 17, 2015, at 2:45 PM, Dan McCombs [email protected] wrote:

In nosepipe.py #7 (comment):

@@ -147,12 +147,16 @@ def call(self, result):
useshell = True

     self.logger.debug("Executing %s", " ".join(argv))
  •    popen = subprocess.Popen(argv,
    
  •                             cwd=self._cwd,
    
  •                             stdout=subprocess.PIPE,
    
  •                             stderr=subprocess.STDOUT,
    
  •                             shell=useshell,
    
  •                             )
    
  •    try:
    
  •        popen = subprocess.Popen(argv,
    
  •                                 cwd=self._cwd,
    
  •                                 stdout=subprocess.PIPE,
    
  •                                 stderr=subprocess.STDOUT,
    
  •                                 shell=useshell,
    
  •                                 )
    
  •    except OSError, e:
    
    Just tried this with Python 3 and noticed this is Python 2 only code - it should instead be except OSError as e:.


Reply to this email directly or view it on GitHub https://github.com/dmccombs/nosepipe/pull/7/files#r24846994.

Copy link
Owner

Choose a reason for hiding this comment

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

I ran into this just running python3 ./setup.py install

Copy link
Owner

Choose a reason for hiding this comment

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

I should note, after installing this I just ran through my normal tests with nosetests to make sure nothing was broken with this change. I don't normally run via setup.py. Does this now fix your issue without problems? I'm confused by your last comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm running with nosetests. I've setup a travis job to show you the failure I'm getting here:

https://travis-ci.org/mdprewitt/nosepipe/jobs/51232640

I'm also messing around with seeing how I can fix this here:

mdprewitt#1

raise Exception("Error running %s [%s]" % (argv[0], e))

try:
stdout = popen.stdout
while True:
Expand Down Expand Up @@ -191,15 +195,31 @@ def __init__(self):
nose.plugins.Plugin.__init__(self)
self._test = None
self._test_proxy = None
self._argv = [os.path.abspath(sys.argv[0]),
'--with-process-isolation-reporter']
self._argv += ProcessIsolationPlugin._get_nose_whitelisted_argv()
nosearg = None
Copy link
Owner

Choose a reason for hiding this comment

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

A comment over this section describing why we're doing this would be good.

for i in range(0, len(sys.argv)):
if 'nosetests' in sys.argv[i]:
self._argv = [sys.argv[i]]
nosearg = i
break

if nosearg is None:
raise Exception("nosetests not found in command-line")

# self._argv = [os.path.abspath(sys.argv[0])]
# if 'nosetests' not in sys.argv[0]:
# for i in range(1, len(sys.argv)):
# self._argv += [sys.argv[i]]
# if 'nosetests' in sys.argv[i]:
# break
Copy link
Owner

Choose a reason for hiding this comment

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

You can just remove the commented code rather than leaving it here commented.


self._argv += ['--with-process-isolation-reporter']
self._argv += ProcessIsolationPlugin._get_nose_whitelisted_argv(nosearg=nosearg)
# Getting cwd inside SubprocessTestProxy.__call__ is too late - it is
# already changed by nose
self._cwd = os.getcwd()

@staticmethod
def _get_nose_whitelisted_argv():
def _get_nose_whitelisted_argv(nosearg=0):
Copy link
Owner

Choose a reason for hiding this comment

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

Could you rename this new argument to something with more meaning in this function - perhaps "offset"? I'd also like to see the +1 added to the argument before passing it in, rather than doing it multiple times inside of here.

# This is the list of nose options which should be passed through to
# the launched process; boolean value defines whether the option
# takes a value or not.
Expand Down Expand Up @@ -228,7 +248,7 @@ def _get_nose_whitelisted_argv():
'--doctest-options': True,
'--no-skip': False,
}
filtered = set(whitelist.keys()).intersection(set(sys.argv[1:]))
filtered = set(whitelist.keys()).intersection(set(sys.argv[nosearg + 1:]))
result = []
for key in filtered:
result.append(key)
Expand All @@ -237,7 +257,7 @@ def _get_nose_whitelisted_argv():

# We are not finished yet: options with '=' were not handled
whitelist_keyval = [(k + "=") for k, v in whitelist.items() if v]
for arg in sys.argv[1:]:
for arg in sys.argv[nosearg + 1:]:
for keyval in whitelist_keyval:
if arg.startswith(keyval):
result.append(arg)
Expand Down
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
license = "BSD",
platforms = ["any"],

install_requires = ["nose>=0.1.0, ==dev"],
install_requires = ["nose>=0.1.0"],

url = "http://github.com/dmccombs/nosepipe/",

Expand Down