CleanCut / green

Green is a clean, colorful, fast python test runner.
MIT License
791 stars 75 forks source link

Using print on Python 2 / Windows causes test failure #134

Closed gurnec closed 7 years ago

gurnec commented 7 years ago

A test case:

import unittest
class Test(unittest.TestCase):
    def test(self):
        print "Test"
        self.assertTrue(True)

Running the above under green v2.5.1 w/Python 2 under Windows (output below) causes 2 failed tests (not sure why it causes 2, that's perhaps another issue).

I tracked down the cause, but I'm not sure where it should be fixed.

Output:

EE

Error in greentest.Test.test
  File "c:\python27\lib\unittest\case.py", line 329, in run
    testMethod()
  File "greentest.py", line 5, in test
    print "Test"
  File "c:\python27\lib\site-packages\green\output.py", line 188, in write
    self.stream.write(text)
  File "c:\python27\lib\site-packages\colorama\ansitowin32.py", line 40, in write
    self.__convertor.write(text)
  File "c:\python27\lib\site-packages\colorama\ansitowin32.py", line 141, in write
    self.write_and_convert(text)
  File "c:\python27\lib\site-packages\colorama\ansitowin32.py", line 169, in write_and_convert
    self.write_plain_text(text, cursor, len(text))
  File "c:\python27\lib\site-packages\colorama\ansitowin32.py", line 174, in write_plain_text
    self.wrapped.write(text[start:end])
TypeError: unicode argument expected, got 'str'

Error in greentest.Test.test
  File "c:\python27\lib\unittest\case.py", line 329, in run
    testMethod()
  File "greentest.py", line 5, in test
    print "Test"
  File "c:\python27\lib\site-packages\green\output.py", line 188, in write
    self.stream.write(text)
  File "c:\python27\lib\site-packages\colorama\ansitowin32.py", line 40, in write
    self.__convertor.write(text)
  File "c:\python27\lib\site-packages\colorama\ansitowin32.py", line 141, in write
    self.write_and_convert(text)
  File "c:\python27\lib\site-packages\colorama\ansitowin32.py", line 169, in write_and_convert
    self.write_plain_text(text, cursor, len(text))
  File "c:\python27\lib\site-packages\colorama\ansitowin32.py", line 174, in write_plain_text
    self.wrapped.write(text[start:end])
TypeError: unicode argument expected, got 'str'

Ran 2 tests in 0.360s

FAILED (errors=2)
CleanCut commented 7 years ago

Hmm. io only exists in Python 3. You are on Python 2. So the import on line 8 will fail and from cStringIO import StringIO will run instead.

That aside, I wonder what's going on.

What exact version of Python and version of colorama are you using? For reference, here's mine:

>>> import sys
>>> sys.version
'2.7.10 (default, Oct 23 2015, 19:19:21) \n[GCC 4.2.1 Compatible Apple LLVM 7.0.0 (clang-700.0.59.5)]'
>>> import colorama
>>> colorama.__version__
'0.3.3'
gurnec commented 7 years ago

Currently I've got Python 2.7.12 (Windows x64 binary downloaded from python.org) and colorama 0.3.7 (via pip, I just installed it today for Green). I'm certain that I've used the io module earlier than 2.7.12 (specifically for its Unicode handling), though I can't remember right now exactly how much earlier.

I also just now checked Ubuntu 14.04 which has Python 2.7.6 by default, and Ubuntu 12.04 which has 2.7.3, and was able on both to import StringIO from io.

I wonder if it's something specific to OS X? I know that its version of Python doesn't include bsddb for example, maybe there are other differences as well?

(in any case, no hurry... and thanks for your work on Green!!)

CleanCut commented 7 years ago

Sorry for the delay. I got super busy at work and then went on vacation. Back now!

What happens if you reverse the imports in suite.py?

Instead of this:

try:
    from io import StringIO
except: # pragma: no cover
    from cStringIO import StringIO

Change it to this:

try:
    from cStringIO import StringIO
except: # pragma: no cover
    from io import StringIO
gurnec commented 7 years ago

Sorry for the delay. I got super busy at work and then went on vacation. Back now!

Certainly no need to apologize, I hope you enjoyed your vaca!

What happens if you reverse the imports in suite.py?

Actually, I've been running this branch which makes these changes instead.

More specifically, I got rid of cStringIO entirely in favor of io.StringIO. According to the docs, the latter has been around since 2.7.0, so assuming you've no need to support 2.6, this simplifies the code a bit.

I also got rid of this line because these two lines already guarantee GreenStream.write() is either passed a unicode, or we convert the passed bytes into a unicode (str.decode('utf-8') always returns a unicode in both 2.7 and 3.x), so the removed line is redundant.

I would have submitted this as a PR earlier, but it's consistently failing unit tests on just one platform (Yosemite). I don't think the failure is related to the changes made, but I've no access to any OS X boxes to troubleshoot.

On Yosemite, I was either seeing the build hang (happened most recently), or an exception from inside multiprocessing somewhere (from memory). I just now tried to restart a build on Yosemite to see if the multiprocessing exception occurred, but apparently Travis is having issues with OS X builds at the moment....

CleanCut commented 7 years ago

Go ahead and submit your PR. I'm not concerned about Yosemite support now that Sierra has been released. Current macOS and one version back is sufficient. One of green's principles is being modern. There are other options for extensive long-term support other than green.

CleanCut commented 7 years ago

(Might as well update the .travis.yml config file in your PR)

gurnec commented 7 years ago

Surprisingly enough, the build which I restarted 2 hours ago for Yosemite just passed (I had tried it several times a few weeks ago, they had all failed). I wonder if the Travis issues were occurring back then?

Would you still like me to update the Travis config, or should I leave it alone?

CleanCut commented 7 years ago

Well, if it's passing we might as well leave it in. Go ahead and add Sierra, though (assuming Travis supports it...)

gurnec commented 7 years ago

It looks like they're getting rid of Xcode 7.1 on Yosemite (but keeping 6.4 on Yosemite) at the end of this month. They're adding Xcode 8 on El Capitan, and plan to switch it to Sierra at some point in the future.

I'll remove 7.1 in the PR, but it may make sense to wait until they've moved Xcode 8 to Sierra before adding it IMHO....