Code review comment for lp:~villemvainio/ipython/ip0-unittests-1

Revision history for this message
Brian Granger (ellisonbg) wrote :

Ville,

> I didn't think it matters - the test case does output to stdout by
> design, since it tests interactive stuff (autocall conversion). I
> don't think it makes sense to redirect stdout here, since nose already
> does it.

What test is failing, I want to have a look.

I don't think we want to get into the habit of allowing tests to write
to stdout...Ideally we will be adding more and more tests of the
interactive stuff, and if more of them do this, our test suite will be
a huge mess when it gets run.

We now actually have quite a few tests and it is really important to
leave no doubts in folks mind what tests are passing and which are
failing. When I see random noise (that doesn't even say what test it
is associated with), things *look* broken, even if there are not. If
we write more tests that have noise on stdout, it will be very
difficult to sort out what is going on. So, I am still not conviced
we want to allow this type of thing.

But, I would like to have a look at this particular test, so if you
could points me in the right direction, that would be great.

> Obviously, it can be done, especially since it's the only test that
> does it so far - but yet it's also among the first tests that
> excercises interactive stuff, and I don't think we should set a
> precedent about not allowing stdout for tests.

I would like to know Fernando's thoughts on this - maybe there is a
way that we can have nose handle this stuff automatically. I would be
fine with that. Fernando would know if this is possible and could
help us implement it.

Cheers,

Brian

> --
> Ville M. Vainio
> http://tinyurl.com/vainio
> https://code.launchpad.net/~villemvainio/ipython/ip0-unittests-1/+merge/1075
> You are subscribed to branch lp:ipython.
>

--
Brian E. Granger, Ph.D.
Assistant Professor of Physics
Cal Poly State University, San Luis Obispo
<email address hidden>
<email address hidden>

« Back to merge proposal