This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author belopolsky
Recipients belopolsky, benjamin.peterson, gpolo, josm, loewis, pitrou, scott.dial
Date 2011-03-11.15:56:20
SpamBayes Score 4.5519144e-15
Marked as misclassified No
Message-id <AANLkTimnMJ0Vswc0SBUXxENempoHVE03ggDtZ3ahjaaW@mail.gmail.com>
In-reply-to <1299655942.95.0.844733066488.issue1706039@psf.upfronthosting.co.za>
Content
On Wed, Mar 9, 2011 at 2:32 AM, Scott Dial <report@bugs.python.org> wrote:
..
> By rejecting unittests on the merits of its coding style, you are creating a double-standard for people like
> me (outside of the core committers), which eventually wears out my interest in helping you get this
> improvement into your project.

I don't think there is a double-standard.  What you see is more of an
evolving standard in which we seek higher quality for the new code
than that of the code already in.  The readability of the test code is
as important if not more important than the readability of the main
code.  It may be more important because in some cases you can trust
obscure code if it is clear that tests cover all corner cases and are
easy to understand.

I don't think there are overly strict standards for how unit tests
should be written, but when they are written in a style that is
unfamiliar to the reviewer, it makes it harder to review.  Note that a
reviewer needs to worry about tests passing on multiple platforms,
their behavior in failure scenarios, as well as other issues that
contributors typically overlook.  Stylistic issues often point out to
real problems, but are easier to spot.  For example, try/finally is
more error prone than with statement.   In fact, in your test
"append" stream will not get closed if writing fails.  You may think
the resource leak on failure is not a big deal, but for some runners
that execute tests repeatedly this may lead to spurious failures.

The acceptance rate for a patch is proportional to the severity of an
issue that it fixes and obviousness of the fix (including tests.)
This patch fails on both counts.  Looking at the history of the issue,
I see that the patch was applied once but reverted because it caused a
crash.  See msg78161.  This shows that the tests were not sufficiently
thorough.  Next, a patch was suggested with tests that did not fail
without a patch.  See msg111516.  This probably shows that additional
tests are needed for 3.2.

These are the reasons I unassigned this issue.  In my opinion it would
take a committer too much effort to bring the patch to commit ready
shape for the marginal benefit of fixing a platform dependent corner
case.  If the tests were better written so that it would be more
obvious what is fixed and that nothing gets broken, my calculation
would probably be different.
History
Date User Action Args
2011-03-11 15:56:22belopolskysetrecipients: + belopolsky, loewis, scott.dial, pitrou, josm, benjamin.peterson, gpolo
2011-03-11 15:56:21belopolskylinkissue1706039 messages
2011-03-11 15:56:20belopolskycreate