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.

classification
Title: Unittest output drives developers to avoid docstrings
Type: Stage: patch review
Components: Tests Versions: Python 3.11
process
Status: open Resolution: works for me
Dependencies: Superseder:
Assigned To: jaraco Nosy List: eric.araujo, ethan.furman, jaraco, methane, miss-islington, terry.reedy
Priority: normal Keywords: patch

Created on 2021-12-18 23:08 by jaraco, last changed 2022-04-11 14:59 by admin.

Pull Requests
URL Status Linked Edit
PR 30194 merged jaraco, 2021-12-19 03:24
PR 32128 merged jaraco, 2022-03-26 15:12
PR 32288 open jaraco, 2022-04-03 15:27
Messages (23)
msg408876 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2021-12-18 23:08
In https://github.com/python/importlib_metadata/issues/302, I learned that the way unittest reports failures in tests is incentivizing the replacement of docstrings with comments in order not to make resolution of the relevant failing test more difficult to locate.

I presume I don't need to explain why docstrings are nice and preferable over comments.

Better would be for unittest to provide an option to ignore the docstrings or to emit the test path regardless of whether a docstring was present and to employ that option in CPython, allowing for docstrings in tests.
msg408878 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2021-12-19 01:01
Could you give a couple examples for those of us not familiar with the problem?
msg408880 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2021-12-19 02:31
Absolutely. I was thinking to do just that.
msg408881 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2021-12-19 02:36
I created this diff:

```diff
diff --git a/Lib/test/test_importlib/test_metadata_api.py b/Lib/test/test_importlib/test_metadata_api.py
index e16773a7e8..92aacd5ad5 100644
--- a/Lib/test/test_importlib/test_metadata_api.py
+++ b/Lib/test/test_importlib/test_metadata_api.py
@@ -90,8 +90,11 @@ def test_entry_points_distribution(self):
             self.assertEqual(ep.dist.version, "1.0.0")
 
     def test_entry_points_unique_packages(self):
-        # Entry points should only be exposed for the first package
-        # on sys.path with a given name.
+        """
+        Entry points should only be exposed for the first package
+        on sys.path with a given name.
+        """
+        raise ValueError("Failing on purpose")
         alt_site_dir = self.fixtures.enter_context(fixtures.tempdir())
         self.fixtures.enter_context(self.add_sys_path(alt_site_dir))
         alt_pkg = {
```

And then ran the tests, but the output is easy to totally scrutable:

```
cpython bpo-46126/bad-error-message $ ./python.exe -m test.test_importlib
........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................s............................E.................................................x......................................................................................................s....................................................................................................................................................................Trying 20 threads ... 44.7 ms OK.
Trying 50 threads ... 36.8 ms OK.
Trying 20 threads ... 27.7 ms OK.
Trying 50 threads ... 28.0 ms OK.
Trying 20 threads ... 27.9 ms OK.
Trying 50 threads ... 31.1 ms OK.
.Trying 20 threads ... 7.1 ms OK.
Trying 50 threads ... 7.6 ms OK.
Trying 20 threads ... 3.2 ms OK.
Trying 50 threads ... 8.5 ms OK.
Trying 20 threads ... 3.4 ms OK.
Trying 50 threads ... 8.7 ms OK.
.Trying 20 threads ... 40.3 ms OK.
Trying 50 threads ... 8.7 ms OK.
Trying 20 threads ... 3.5 ms OK.
Trying 50 threads ... 6.5 ms OK.
Trying 20 threads ... 3.2 ms OK.
Trying 50 threads ... 6.5 ms OK.
..................................................s............................s................................................s............................s...............
======================================================================
ERROR: test_entry_points_unique_packages (test.test_importlib.test_metadata_api.APITests)
Entry points should only be exposed for the first package
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jaraco/code/public/cpython/Lib/test/test_importlib/test_metadata_api.py", line 97, in test_entry_points_unique_packages
    raise ValueError("Failing on purpose")
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: Failing on purpose

----------------------------------------------------------------------
Ran 1426 tests in 2.377s

FAILED (errors=1, skipped=6, expected failures=1)
```

So there must be some other test invocation that doesn't provide the clarity of which test is failing.
msg408882 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2021-12-19 02:43
My guess is the tests were run with something like this:

```
./python.exe -E -We -m test -v test_importlib | grep '... ERROR'
Entry points should only be exposed for the first package ... ERROR
test test_importlib failed
```

In that case, the location of the test is replaced with the first line of the docstring. If there's no docstring, however, the location of the failure is easier to detect:

```
$ ./python.exe -E -We -m test -v test_importlib | grep '... ERROR'
test_entry_points_unique_packages (test.test_importlib.test_metadata_api.APITests) ... ERROR
test test_importlib failed
```
msg408883 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2021-12-19 02:53
Looks like the code, I believe here's where the reporting happens: https://github.com/python/cpython/blob/9b52920173735ac609664c6a3a3021d24a95a092/Lib/unittest/runner.py#L48-L51

I see a "description" property there that may already provide the feature.
msg408884 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2021-12-19 02:59
It looks like that 'descriptions' attribute is passed through from TextTestRunner, which sets it to True by default (https://github.com/python/cpython/blob/9b52920173735ac609664c6a3a3021d24a95a092/Lib/unittest/runner.py#L163). It seems that behavior isn't controllable through any command-line options or environment variables.
msg408885 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2021-12-19 03:22
After some investigation (and tracing calls through a dozen or more layers), I found that Python's own regression tests can disable this behavior thus:

```
diff --git a/Lib/test/support/testresult.py b/Lib/test/support/testresult.py
index 2cd1366cd8..328ca8760e 100644
--- a/Lib/test/support/testresult.py
+++ b/Lib/test/support/testresult.py
@@ -145,7 +145,8 @@ def get_test_runner_class(verbosity, buffer=False):
         return functools.partial(unittest.TextTestRunner,
                                  resultclass=RegressionTestResult,
                                  buffer=buffer,
-                                 verbosity=verbosity)
+                                 verbosity=verbosity,
+                                 descriptions=False,)
     return functools.partial(QuietRegressionTestRunner, buffer=buffer)
 
 def get_test_runner(stream, verbosity, capture_output=False):
```

Combined with the above diff (with a docstring), the output is now as desired:

```
$ ./python.exe -m test -v test_importlib | grep '... ERROR'
test_entry_points_unique_packages (test.test_importlib.test_metadata_api.APITests) ... ERROR
test test_importlib failed
```

It seems it would be wise to enable this setting by default, given that without it, every test module is forced to replace docstrings with comments.
msg409159 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2021-12-24 22:29
> I presume I don't need to explain why docstrings are nice and preferable over comments.

Actually, can you?

Docstrings to document regular modules, classes or functions are a fine tool, especially with tools that extract them (pydoc, sphinx).  I don’t see the same need for test functions.
msg409161 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2021-12-24 22:52
I also use comments in lieu of docstrings for text_xyx methods because I find the addition of the first line of the docstring in error reports to be useless, distracting, and sometimes, depending on the content, confusing.  If the error is in the test method, the full comment is available when looking back at the method code.  If the error is in the tested code, the comment/docstring is inapplicable.

When I edit test files, I run them directly from an IDLE editor via the 'if __name__' clause.  When I run all IDLE tests from Command Prompt, I usually run 'python -m test.test_idle'.  (Similar but not necessarily identical to 'python -m -ugui test_idle'.)  So fiddling with regrtest will not help in either case.

Based on the above, the following seems to work.
    runner = unittest.TextTestRunner(descriptions=False, verbosity=2)    
    unittest.main(testRunner=runner)
I am not sure what passing a runner does, versus leaving the default None, but the verbosity passed to the runner overrides and verbosity argument passed to main.  What I would like is 'descriptions' added as a parameter to main, so that
    unittest.main(descriptions=False, verbosity=2)
would work.
msg409478 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2022-01-01 20:37
> > I presume I don't need to explain why docstrings are nice and preferable over comments.

> Actually, can you?

I searched around and didn't find any good treatise or thorough overview  of reasons _why_ docstrings should be preferred, so I performed an internal deconstruction of my motivations and published [this article](https://blog.jaraco.com/why-docstrings-are-preferable-to-comments/) on my blog.
msg411279 - (view) Author: miss-islington (miss-islington) Date: 2022-01-22 18:49
New changeset a941e5927f7f2540946813606c61c6aea38db426 by Jason R. Coombs in branch 'main':
bpo-46126: Disable 'descriptions' when running tests internally. (GH-30194)
https://github.com/python/cpython/commit/a941e5927f7f2540946813606c61c6aea38db426
msg411280 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2022-01-22 18:52
I've merged the fix for regrtest and I'll explore Terry's concerns and see what I can devise for those concerns as well.
msg415645 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2022-03-21 00:42
In an attempt to replicate Terry's usage, I added the following patch:

```
diff --git a/Lib/idlelib/idle_test/test_text.py b/Lib/idlelib/idle_test/test_text.py
index 0f31179e04..be977bbfff 100644
--- a/Lib/idlelib/idle_test/test_text.py
+++ b/Lib/idlelib/idle_test/test_text.py
@@ -20,6 +20,10 @@ def test_init(self):
         self.assertEqual(self.text.get('end'), '')
 
     def test_index_empty(self):
+        """
+        Failing test with bad description.
+        """
+        raise ValueError()
         index = self.text.index
 
         for dex in (-1.0, 0.3, '1.-1', '1.0', '1.0 lineend', '1.end', '1.33',
```

When I did, I then ran the tests using the `-m` launcher:

```
$ ./python.exe -m test.test_idle -v -k test_index_empty
idlelib.idle_test.test_configdialog (unittest.loader.ModuleSkipped) ... skipped 'cannot run without OS X gui process'
idlelib.idle_test.test_debugger (unittest.loader.ModuleSkipped) ... skipped 'cannot run without OS X gui process'
idlelib.idle_test.test_editmenu (unittest.loader.ModuleSkipped) ... skipped 'cannot run without OS X gui process'
idlelib.idle_test.test_help (unittest.loader.ModuleSkipped) ... skipped 'cannot run without OS X gui process'
idlelib.idle_test.test_parenmatch (unittest.loader.ModuleSkipped) ... skipped 'cannot run without OS X gui process'
idlelib.idle_test.test_percolator (unittest.loader.ModuleSkipped) ... skipped 'cannot run without OS X gui process'
idlelib.idle_test.test_replace (unittest.loader.ModuleSkipped) ... skipped 'cannot run without OS X gui process'
idlelib.idle_test.test_scrolledlist (unittest.loader.ModuleSkipped) ... skipped 'cannot run without OS X gui process'
idlelib.idle_test.test_search (unittest.loader.ModuleSkipped) ... skipped 'cannot run without OS X gui process'
test_index_empty (idlelib.idle_test.test_text.MockTextTest)
Failing test with bad description. ... ERROR
setUpClass (idlelib.idle_test.test_text.TkTextTest) ... skipped 'cannot run without OS X gui process'
idlelib.idle_test.test_textview (unittest.loader.ModuleSkipped) ... skipped 'cannot run without OS X gui process'
idlelib.idle_test.test_tooltip (unittest.loader.ModuleSkipped) ... skipped 'cannot run without OS X gui process'
idlelib.idle_test.test_tree (unittest.loader.ModuleSkipped) ... skipped 'cannot run without OS X gui process'
idlelib.idle_test.test_undo (unittest.loader.ModuleSkipped) ... skipped 'cannot run without OS X gui process'

======================================================================
ERROR: test_index_empty (idlelib.idle_test.test_text.MockTextTest)
Failing test with bad description.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jaraco/code/public/cpython/Lib/idlelib/idle_test/test_text.py", line 26, in test_index_empty
    raise ValueError()
    ^^^^^^^^^^^^^^^^^^
ValueError

----------------------------------------------------------------------
Ran 14 tests in 0.001s

FAILED (errors=1, skipped=14)
```

As you can see, the location of the failing test in the log is masked, and instead the description is present.

I've pushed https://github.com/python/cpython/tree/bpo-46126/disable-descriptions-unittest-m, which when applied disables the descriptions.

This approach works as a surgical intervention, wrapping `unittest.main` in such a way that it could be replaced across the test suite to disable descriptions. By subclassing the TextTestRunner, it continues to honor other parameters from the command-line (such as verbosity).

Terry, would you review the concept? Does it meet your needs? If so, I can apply it more broadly and prepare a PR.
msg415649 - (view) Author: Inada Naoki (methane) * (Python committer) Date: 2022-03-21 02:52
> As you can see, the location of the failing test in the log is masked, and instead the description is present.

Could you elaborate?

```
test_index_empty (idlelib.idle_test.test_text.MockTextTest)
Failing test with bad description. ... ERROR
(snip)
======================================================================
ERROR: test_index_empty (idlelib.idle_test.test_text.MockTextTest)
Failing test with bad description.
----------------------------------------------------------------------
```

I can see `test_index_empty (idlelib.idle_test.test_text.MockTextTest)` in both places. What is masked?
msg415826 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2022-03-22 22:09
Inada, you're right. Good catch. I think I missed that behavior from before because I used `grep ERROR` and when I ran it this time, I just assumed the output would be the same, but it's clear that even _with descriptions enabled_, the location of the test is clearly reported in the case of launching with `-m unittest`.

I'm now suspicious that my report in https://bugs.python.org/issue46126#msg408882 was also flawed as it uses `grep ERROR`, which could easily mask lines that adjacent to the error.

I now believe that there are no concerns relating to Terry's concern, and likely not Brett's concern despite my best efforts to replicate. I'll bring back the original example and see if it in fact has the reported defect.
msg415834 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2022-03-22 22:32
And indeed, after removing the `grep ERROR` part of the repro, even the repro seems to be invalid:

```
cpython main $ ./python.exe -m test.test_importlib -v -k test_entry_points_unique
test_entry_points_unique_packages (test.test_importlib.test_metadata_api.APITests)
Entry points should only be exposed for the first package ... ERROR
test.test_importlib.test_windows (unittest.loader.ModuleSkipped) ... skipped "No module named 'winreg'"

======================================================================
ERROR: test_entry_points_unique_packages (test.test_importlib.test_metadata_api.APITests)
Entry points should only be exposed for the first package
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jaraco/code/public/cpython/Lib/test/test_importlib/test_metadata_api.py", line 97, in test_entry_points_unique_packages
    raise ValueError("Failing on purpose")
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: Failing on purpose

----------------------------------------------------------------------
Ran 2 tests in 0.006s

FAILED (errors=1, skipped=1)
```

I'm effectively unable to replicate the behavior reported by Brett in the other issue (unless `| grep ERROR` is consider an important use-case to support).

At this point, I propose one of two options:

1. Back out PR 30194 and restore the prior behavior with descriptions.
2. Leave PR 30194 to suppress descriptions for the default runner.

And then simply declare that docstrings are acceptable for CPython tests.

Given that descriptions are on by default, I'm slightly inclined toward (1).

Thoughts?
msg415870 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2022-03-23 13:13
I think the situation and the discussion should be summarized on python-dev!
msg416068 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2022-03-26 15:05
I've confirmed that prior to the patch in PR 30194, the location of the failure was indeed reported. It was just not reported on the same line:

```
cpython main $ git log -1
commit 3e93af0b06cada874c4a16868b6f863b599919f2 (HEAD -> main)
Author: Jason R. Coombs <jaraco@jaraco.com>
Date:   Sat Mar 26 10:45:58 2022 -0400

    Revert "bpo-46126: Disable 'descriptions' when running tests internally. (GH-30194)"
    
    This reverts commit a941e5927f7f2540946813606c61c6aea38db426.
cpython main $ git diff
diff --git a/Lib/test/test_importlib/test_metadata_api.py b/Lib/test/test_importlib/test_metadata_api.py
index b3627cbb75..7e3bb09cf2 100644
--- a/Lib/test/test_importlib/test_metadata_api.py
+++ b/Lib/test/test_importlib/test_metadata_api.py
@@ -90,8 +90,11 @@ def test_entry_points_distribution(self):
             self.assertEqual(ep.dist.version, "1.0.0")
 
     def test_entry_points_unique_packages(self):
-        # Entry points should only be exposed for the first package
-        # on sys.path with a given name.
+        """
+        Entry points should only be exposed for the first package
+        on sys.path with a given name.
+        """
+        raise ValueError("Failing on purpose")
         alt_site_dir = self.fixtures.enter_context(fixtures.tempdir())
         self.fixtures.enter_context(self.add_sys_path(alt_site_dir))
         alt_pkg = {
cpython main $ ./python.exe -E -We -m test -v test_importlib | grep entry_points_unique_packages
test_entry_points_unique_packages (test.test_importlib.test_metadata_api.APITests)
ERROR: test_entry_points_unique_packages (test.test_importlib.test_metadata_api.APITests)
  File "/Users/jaraco/code/public/cpython/Lib/test/test_importlib/test_metadata_api.py", line 97, in test_entry_points_unique_packages
test test_importlib failed
```

The description is given _in addition_ to the location of the test.

That means the concern reported in #302 was actually invalid.

I also was under the false impression that the description was masking the test location, but instead, it's just enhancing it (while also injecting a newline).

Given this renewed understanding, I believe it's appropriate to back out the PR.

> I think the situation and the discussion should be summarized on python-dev!

Great suggestion. Will do.
msg416081 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2022-03-26 19:09
Perhaps we could make an enhancement then?  Having the extra information on a separate line is, at least for me, very jarring -- as in, I hadn't figured out that that was the way it was done until Inadasan pointed it out.

Perhaps a command-line switch to enable having it all on one line?  That would also help when grepping.  (I would propose it to be the default behavior, but I can see that that would result in very long lines.)
msg416088 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2022-03-26 20:26
My goal with this issue is to simply unblock the use of docstrings in Python tests. I believe with the work above and the proposed published post, I've accomplished that goal. I've already spent more time than I'd hoped on this issue and would like to resolve it at its original scope.

I do see how this issue reveals some deficiencies with the current UI/UX, and I'd welcome a separate feature request to address those deficiencies. I'll even help draft the bug report on request.
msg416091 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2022-03-26 20:57
That makes sense.

issue47133 created.
msg416641 - (view) Author: miss-islington (miss-islington) Date: 2022-04-03 19:33
New changeset 84acb5cad1b871bb8729cbf1d036b84cbe1636f0 by Jason R. Coombs in branch 'main':
bpo-46126: Restore 'descriptions' when running tests internally. (GH-32128)
https://github.com/python/cpython/commit/84acb5cad1b871bb8729cbf1d036b84cbe1636f0
History
Date User Action Args
2022-04-11 14:59:53adminsetgithub: 90284
2022-04-03 19:33:37miss-islingtonsetmessages: + msg416641
2022-04-03 15:27:29jaracosetpull_requests: + pull_request30349
2022-03-26 20:57:02ethan.furmansetmessages: + msg416091
2022-03-26 20:26:00jaracosetmessages: + msg416088
2022-03-26 19:09:40ethan.furmansetmessages: + msg416081
2022-03-26 15:12:25jaracosetpull_requests: + pull_request30208
2022-03-26 15:11:01jaracosetresolution: works for me
2022-03-26 15:05:53jaracosetmessages: + msg416068
2022-03-23 13:13:02eric.araujosetmessages: + msg415870
2022-03-22 22:32:23jaracosetmessages: + msg415834
2022-03-22 22:09:19jaracosetassignee: terry.reedy -> jaraco
messages: + msg415826
2022-03-21 02:52:14methanesetnosy: + methane
messages: + msg415649
2022-03-21 00:42:30jaracosetassignee: jaraco -> terry.reedy
messages: + msg415645
2022-01-22 18:52:44jaracosetassignee: jaraco
2022-01-22 18:52:39jaracosetmessages: + msg411280
2022-01-22 18:49:46miss-islingtonsetnosy: + miss-islington
messages: + msg411279
2022-01-01 20:37:05jaracosetmessages: + msg409478
2021-12-25 07:24:28terry.reedysetnosy: + eric.araujo
2021-12-24 22:52:24terry.reedysetnosy: + terry.reedy, - eric.araujo
messages: + msg409161
2021-12-24 22:29:09eric.araujosetnosy: + eric.araujo
messages: + msg409159
2021-12-19 03:24:05jaracosetkeywords: + patch
stage: patch review
pull_requests: + pull_request28415
2021-12-19 03:22:25jaracosetmessages: + msg408885
2021-12-19 02:59:04jaracosetmessages: + msg408884
2021-12-19 02:53:06jaracosetmessages: + msg408883
2021-12-19 02:43:30jaracosetmessages: + msg408882
2021-12-19 02:36:31jaracosetmessages: + msg408881
2021-12-19 02:31:03jaracosetmessages: + msg408880
2021-12-19 01:01:38ethan.furmansetnosy: + ethan.furman
messages: + msg408878
2021-12-18 23:08:24jaracocreate