classification
Title: Add tests for some scripts in Tools/scripts
Type: enhancement Stage: resolved
Components: Demos and Tools, Tests Versions: Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: eric.araujo Nosy List: eric.araujo, eric.snow, ezio.melotti, francismb, mark.dickinson, nadeem.vawda, python-dev, tshepang
Priority: normal Keywords: patch

Created on 2011-11-21 17:08 by eric.araujo, last changed 2012-02-26 03:20 by eric.araujo. This issue is now closed.

Files
File name Uploaded Description Edit
add-test_tools.diff eric.araujo, 2012-02-22 12:55 review
Messages (19)
msg148062 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-11-21 17:08
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.
msg148064 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-11-21 17:18
+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).
msg153811 - (view) Author: Francis MB (francismb) * Date: 2012-02-20 21:31
+1

while working on #issue14053 I missed the test for the scripts (in this case some kind of mocking is needed).
msg153818 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-20 23:49
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.
msg153953 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-22 12:55
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.
msg153978 - (view) Author: Francis MB (francismb) * Date: 2012-02-22 18:01
> 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) …
msg154021 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-23 00:28
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)

:)
msg154023 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2012-02-23 00:43
> 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).
msg154024 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-23 00:45
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 :)
msg154071 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2012-02-23 16:55
> 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 ;-)
msg154197 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-25 07:29
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?
msg154203 - (view) Author: Francis MB (francismb) * Date: 2012-02-25 08:06
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
msg154209 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2012-02-25 08:21
[É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.
msg154210 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-25 08:25
> 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.
msg154212 - (view) Author: Francis MB (francismb) * Date: 2012-02-25 08:34
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...)
msg154260 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2012-02-25 15:25
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
msg154266 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-25 16:00
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?
msg154303 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2012-02-26 03:07
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
msg154306 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-26 03:20
> 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.
History
Date User Action Args
2012-02-26 03:20:39eric.araujosetstatus: open -> closed
resolution: fixed
messages: + msg154306

stage: test needed -> resolved
2012-02-26 03:07:49python-devsetmessages: + msg154303
2012-02-25 16:00:17eric.araujosetnosy: + mark.dickinson
messages: + msg154266
2012-02-25 15:25:08python-devsetnosy: + python-dev
messages: + msg154260
2012-02-25 08:34:18francismbsetmessages: + msg154212
2012-02-25 08:25:21eric.araujosetmessages: + msg154210
2012-02-25 08:21:47nadeem.vawdasetmessages: + msg154209
2012-02-25 08:06:15francismbsetmessages: + msg154203
2012-02-25 07:29:02eric.araujosetmessages: + msg154197
2012-02-23 16:55:16nadeem.vawdasetmessages: + msg154071
2012-02-23 00:45:47eric.araujosetmessages: + msg154024
2012-02-23 00:43:13ezio.melottisetmessages: + msg154023
2012-02-23 00:28:17eric.araujosetmessages: + msg154021
2012-02-22 18:01:03francismbsetmessages: + msg153978
2012-02-22 16:57:15eric.snowsetnosy: + eric.snow
2012-02-22 12:55:14eric.araujosetfiles: + add-test_tools.diff
keywords: + patch
messages: + msg153953
2012-02-22 12:45:39tshepangsetnosy: + tshepang
2012-02-22 12:43:37nadeem.vawdasetnosy: + nadeem.vawda
2012-02-20 23:49:48eric.araujosetassignee: eric.araujo
messages: + msg153818
2012-02-20 21:31:21francismbsetmessages: + msg153811
2012-02-20 17:24:59ezio.melottisetnosy: + francismb
type: enhancement
2012-01-16 16:36:44eric.araujolinkissue8502 dependencies
2012-01-13 17:20:12eric.araujounlinkissue1475523 dependencies
2012-01-13 17:19:58eric.araujolinkissue1475523 dependencies
2012-01-13 17:17:09eric.araujosettitle: Add tests for Tools/scripts/reindent.py -> Add tests for some scripts in Tools/scripts
2011-11-21 17:18:20ezio.melottisetnosy: + ezio.melotti

messages: + msg148064
stage: test needed
2011-11-21 17:10:55eric.araujolinkissue12930 dependencies
2011-11-21 17:08:03eric.araujocreate