When people find bugs in reindent.py (#12930 is a recent one), we have to try to reproduce the bug manually, apply a fix and test manually again.  The alternative is to only read the code and trust that it works.  I find both of these positions unsatisfactory and propose to add unit tests for the script.  I’m opening this report to gather votes, and if they’re positive I’ll proceed with a patch and other bug reports for the other most used scripts (we have a number of reports for pygettext and msgfmt for example; I am not proposing adding tests for all, or even most, scripts).

Even if these scripts are not part of the stdlib and not officially documented or supported, we do ship them and use them (reindent from patchcheck and Mercurial hooks for example), so I really think we need unit tests to know that they work as intended.  Then we’ll be able to commit fixes and other changes serenely.

Implementation idea: The tests would go in Tools/scripts/tests/test_reindent.py and a short file in Lib/test/test_scripts.py would run them as part of the test suite if they exist (so that running tests from an installed Python without Tools dir just skips test_scripts).  For scripts that are in another Tools subdir, like Tools/i18n/pygettext.py, I’d put them in Tools/i18n/tests/test_pygettext.py.

+1

What you describe sounds all good to me, except that I would keep all tests in the same dir (i.e. Tools/tests (or Tools/test?)) instead of having different test dirs for each subdir (so test_pygettext.py would go in Tools/tests and not in Tools/i18n/tests).

+1

while working on #issue14053 I missed the test for the scripts (in this case some kind of mocking is needed).

I’m pushing this higher on my todo list then.  I’ll certainly upload a patch for review here with the basic machinery, then commit it, and then we’ll be able to add tests for the tools we like.

I went for something even simpler: one new file Lib/test/test_tools.py.  There is no need to have a file there and more files in Tools/tests, everything can go in test_tools (I’m not worried about file size, we already have a few thousand-liners test modules).  I think it’s the simplest thing that works.

My only concern is communication: how do we tell people working on a tool that they should write a test in test_tools?  I’m not sure they would read Tools/README; maybe a note at the top of the Python files would work; a note in the devguide; sending an email to python-dev to tell about the new file + reminders when we see a new tool bug opened.

> My only concern is communication: how do we tell people working on a tool that they should write a test in test_tools?  I’m not sure they would read Tools/README; maybe a note at the top of the Python files would work; a note in the devguide; sending an email to python-dev to tell about the new file + reminders when we see a new tool bug opened.

Patch patchcheck to "check" ;-) if some file in Tool/** has been touched 
and give a message accordingly (that happens already for NEWS and ACKS) …

Nice idea, but

1) not everyone uses patchcheck

2) patchcheck comes after you’ve fixed something, but tests should be written before fixes (to reproduce the error)

:)

> I went for something even simpler: one new file Lib/test/test_tools.py.

I'm fine with this option too; if it grows too big we can always split it in several modules later.

> My only concern is communication: how do we tell people working on a
> tool that they should write a test in test_tools?

I don't think it's a big deal, eventually people will learn that it's there and we can always point to that file when a new issue is opened or during the patch review.
Adding a check to `make patchcheck` will also help (it's still better to add tests later than leaving the code untested), but maybe it should be displayed only if something is actually changed in the Tools dir (since that doesn't happen often, having always a "Tools modified... no" will just add noise to the output).

Shall I commit the new file or first add more tests?

> I don't think it's a big deal, eventually people will learn that it's there and we can always
> point to that file when a new issue is opened or during the patch review.
Good.

> Adding a check to `make patchcheck` will also help [...]
I’ll let Francisco do that in another bug report :)

> Shall I commit the new file or first add more tests?

Might as well commit now; there's no sense in leaving the code sitting
around just because the file feels a bit short.

A couple of minor nits about your patch, though:

- The docstring for test_tools says "Tests for reindent.py". It would be
  better to have something referring to the Tools directory, so people
  know that this is the right place to put new tests for scripts that
  currently don't have any.

- The wording of the message 'requires an uninstalled Python build' is a
  bit awkward - I'd use "non-installed" instead of "uninstalled", so
  inexperienced users don't go and try to uninstall their system Python ;-)

Good points.

New docstring:

  Tests for scripts in the Tools directory.

  This file contains regression tests for some of the scripts found in the
  Tools directory of a Python checkout or tarball, such as reindent.py.

When I commit I’ll also send to python-dev a reply to the python-checkins email to advertise the new file.

New skip message:

  test irrelevant for an installed Python

Better?

Hi Éric,
some questions:

1)if test_tools is going to be the test for all the Tools (at least
until it grows to much), shoudn't be be module doc something like “””Tests for scripts in Tools/**“””

2)is the SkipTest “reindent” specific? 

Cheers,

francis

[Éric]
> New docstring:
>
>   Tests for scripts in the Tools directory.
>
>   This file contains regression tests for some of the scripts found in the
>   Tools directory of a Python checkout or tarball, such as reindent.py.
>
> When I commit I’ll also send to python-dev a reply to the python-checkins email to advertise the new file.
>
> New skip message:
>
>   test irrelevant for an installed Python
>
> Better?

Sounds good to me.

[Francisco]
> 1)if test_tools is going to be the test for all the Tools (at least
> until it grows to much), shoudn't be be module doc something like “””Tests for scripts in Tools/**“””

I think Éric's suggestion in msg154197 is fine.

> 2)is the SkipTest “reindent” specific? 

No, it's common to all of the Tools scripts. When you install Python, the
Tools directory is not copied into the install destination. So if you're
using an installed copy of Python (either from running "make install", or
from a binary installer), the scripts won't exist. In this case, we
(obviously) can't test them.

> if test_tools is going to be the test for all the Tools (at least until it grows to much),
Seriously, we don’t care about the size of test files.  test_argparse.py is 4777 lines long :)

> shoudn't be be module doc something like “””Tests for scripts in Tools/**“””
I find this wording worse than mine, sorry.

> is the SkipTest “reindent” specific? 
The SkipTest is raised at the top level of the module.  No test from test_tools will be able to run outside of an uninstalled build.  Maybe you are asking that because my message uses the singular, i.e. “test irrelevant etc.” and not “tests”?  In this instance, the singular “test” refers to test_tools, viewed as one test.

Sorry, my fault: I meant "Test for Tools" instead of """Tests for 
reindent.py.""" (Im not talking about the skip message but the test 
documentation or the first line 0,0...)

New changeset 020364d3e359 by Éric Araujo in branch '2.7':
Add test file for scripts in Tools (#13447).
http://hg.python.org/cpython/rev/020364d3e359

Hi Mark.  You’re the author of Tools/parser/test_unparse.py; any objection if I move it to the new Lib/test/test_tools.py file, so that all tests for Tools are in one place?

New changeset 2887f5a97075 by Éric Araujo in branch '3.2':
Add test file for scripts in Tools (#13447).
http://hg.python.org/cpython/rev/2887f5a97075

> I meant "Test for Tools" instead of """Tests for reindent.py.""" 
Looks like you missed the message where I wrote how I changed s/reindent.py/scripts in the Tools directory/

Committed!  Now we can tackle all these nasty pygettext and msgfmt bugs, and I will also add tests for the fixes that were done the last months.