Issue46126
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.
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) * ![]() |
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) * ![]() |
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) * ![]() |
Date: 2021-12-19 02:31 | |
Absolutely. I was thinking to do just that. |
|||
msg408881 - (view) | Author: Jason R. Coombs (jaraco) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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:53 | admin | set | github: 90284 |
2022-04-03 19:33:37 | miss-islington | set | messages: + msg416641 |
2022-04-03 15:27:29 | jaraco | set | pull_requests: + pull_request30349 |
2022-03-26 20:57:02 | ethan.furman | set | messages: + msg416091 |
2022-03-26 20:26:00 | jaraco | set | messages: + msg416088 |
2022-03-26 19:09:40 | ethan.furman | set | messages: + msg416081 |
2022-03-26 15:12:25 | jaraco | set | pull_requests: + pull_request30208 |
2022-03-26 15:11:01 | jaraco | set | resolution: works for me |
2022-03-26 15:05:53 | jaraco | set | messages: + msg416068 |
2022-03-23 13:13:02 | eric.araujo | set | messages: + msg415870 |
2022-03-22 22:32:23 | jaraco | set | messages: + msg415834 |
2022-03-22 22:09:19 | jaraco | set | assignee: terry.reedy -> jaraco messages: + msg415826 |
2022-03-21 02:52:14 | methane | set | nosy:
+ methane messages: + msg415649 |
2022-03-21 00:42:30 | jaraco | set | assignee: jaraco -> terry.reedy messages: + msg415645 |
2022-01-22 18:52:44 | jaraco | set | assignee: jaraco |
2022-01-22 18:52:39 | jaraco | set | messages: + msg411280 |
2022-01-22 18:49:46 | miss-islington | set | nosy:
+ miss-islington messages: + msg411279 |
2022-01-01 20:37:05 | jaraco | set | messages: + msg409478 |
2021-12-25 07:24:28 | terry.reedy | set | nosy:
+ eric.araujo |
2021-12-24 22:52:24 | terry.reedy | set | nosy:
+ terry.reedy, - eric.araujo messages: + msg409161 |
2021-12-24 22:29:09 | eric.araujo | set | nosy:
+ eric.araujo messages: + msg409159 |
2021-12-19 03:24:05 | jaraco | set | keywords:
+ patch stage: patch review pull_requests: + pull_request28415 |
2021-12-19 03:22:25 | jaraco | set | messages: + msg408885 |
2021-12-19 02:59:04 | jaraco | set | messages: + msg408884 |
2021-12-19 02:53:06 | jaraco | set | messages: + msg408883 |
2021-12-19 02:43:30 | jaraco | set | messages: + msg408882 |
2021-12-19 02:36:31 | jaraco | set | messages: + msg408881 |
2021-12-19 02:31:03 | jaraco | set | messages: + msg408880 |
2021-12-19 01:01:38 | ethan.furman | set | nosy:
+ ethan.furman messages: + msg408878 |
2021-12-18 23:08:24 | jaraco | create |