yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Review: Approve

Wow!  Great work.  Nice changes to the tests, and thank you for cleaning up the world.

I have two comments: one each about the most predictably commentable things in the MP.

(1) For get_real_stdout and friend, would you feel better and be able to remove that apologetic comment if the code looked like this instead?

___stdout = ___stderr = None
def get_real_stdout():
    """The canonical right place to get __stdout__."""
    return ___stdout or sys.__stdout__
def get_real_stderr():
    """The canonical right place to get __stderr__."""
    return ___stderr or sys.__stderr__
def set_stdout(new):
    """Set __stdout__ so that you can clean things up later.
    Other code can get at the original value with get_real_stdout."""
    global ___stdout
    ___stdout = sys.__stdout__
    sys.__stdout__ = new
def set_stderr(new):
    """Set __stderr__ so that you can clean things up later.
    Other code can get at the original value with get_real_stderr."""
    global ___stderr
    ___stderr = sys.__stderr__
    sys.__stderr__ = new
def reset_stdout():
    global ___stdout
    result = sys.__stdout__
    sys.__stdout__ = ___stdout
    ___stdout = None
    return result
def reset_stderr():
    global ___stderr
    result = sys.__stderr__
    sys.__stderr__ = ___stderr
    ___stderr = None
    return result

Maybe you want those to mess with stdout and stderr too.

Then, in testrunner.options.get_options, instead of directly mucking with __stderr__...

                # If we are running in a subprocess then the test runner code will
                # use stderr as a communication channel back to the spawning
                # process so we shouldn't touch it.
                if '--resume-layer' not in sys.argv:
                    sys.__stderr__ = sys.stderr = StringIO()

...we say "zope.testing.testrunner.set_stderr(StringIO())"

Similarly, when we set __stdout__ in doctests, we use the functions above, and then reset them using the functions.

The advantage is that we are not doing anything at __init__ time, and everything seems a bit cleaner to me.  What do you think?

(2) For the new XXX in src/zope/testing/testrunner/tests.py in which we disable the tests, please add a bug for the broken tests and mention the number.  I think it's worth following the usual practice.

Thanks again!
-- 
https://code.launchpad.net/~yellow/zope.testing/fix-tests/+merge/111442
Your team Yellow Squad is requested to review the proposed merge of lp:~yellow/zope.testing/fix-tests into lp:~launchpad/zope.testing/3.9.4-fork.