bpo-37501: Fix test failures when CPython is built without docstrings #14592
Conversation
| @@ -10,6 +10,7 @@ | |||
| import importlib.util | |||
| import importlib | |||
| from test.support.script_helper import assert_python_failure | |||
| from test.support import HAVE_DOCSTRINGS, MISSING_C_DOCSTRINGS | |||
There was a problem hiding this comment.
Is importing MISSING_C_DOCSTRINGS needed?
| @@ -858,6 +859,7 @@ def bar(self) -> int: | |||
| self.assertEqual(C(10).bar, 5) | |||
| self.assertEqual(C(10).i, 10) | |||
|
|
|||
| @requires_docstrings | |||
| def test_post_init(self): | |||
There was a problem hiding this comment.
I am not sure that this test tests only docstrings.
There was a problem hiding this comment.
I don't see how it tests doc strings at all. If there's some failure related to lack of docstrings, this will need to be refactored so that the non-docstring-related parts (if any) are still executed.
|
@ZackerySpytz Did you verify that tests pass after patch on no-docstrings build, or with -OO? |
There was a problem hiding this comment.
Changes to test_dataclasses.py, if valid, need to refactor the tests so as to not remove code that doesn't depend on docstrings. But I don't see how the marked tests depend on docstrings. Maybe a refactoring will help make that more obvious. Also, if these tests do depend on docstrings, please add comments describing how the tests exercise docstrings.
| @@ -858,6 +859,7 @@ def bar(self) -> int: | |||
| self.assertEqual(C(10).bar, 5) | |||
| self.assertEqual(C(10).i, 10) | |||
|
|
|||
| @requires_docstrings | |||
| def test_post_init(self): | |||
There was a problem hiding this comment.
I don't see how it tests doc strings at all. If there's some failure related to lack of docstrings, this will need to be refactored so that the non-docstring-related parts (if any) are still executed.
| @@ -2976,6 +2978,7 @@ def test_class_var(self): | |||
| self.assertEqual(C.y, 10) | |||
| self.assertEqual(C.z, 20) | |||
|
|
|||
| @requires_docstrings | |||
There was a problem hiding this comment.
I also don't see how this tests docstrings, either.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
| self.assertEqual(module.__doc__, "Module named in %s" % lang) | ||
| if HAVE_DOCSTRINGS: | ||
| self.assertEqual(module.__doc__, | ||
| "Module named in %s" % lang) |
There was a problem hiding this comment.
For readability and consistency, I would recommend replacing the old string formatting with f strings:
"Module named in %s" % lang #current
f"Module named in {lang}" #suggestionThis does not make a functional difference, but since this section is being modified (and it's the only section in the file which is using the old string formatting) it may as well be updated to use f strings instead.
|
@ZackerySpytz, please address the code reviews. Thank you! |
There was a problem hiding this comment.
IDLE produces calltips with or without docstrings present. Calltips tests should always run. I opened bpo-39456 for this. If I merge a patch for that before this one is merged, then test_calltip MUST be removed from this one. Skipping is at best a short-term workaround.
|
It is pretty old PR, and it is easier to recreate it from scratch than resolve merge conflicts. Especially if there are not answered questions. Also, many new tests were added. So I created a new PR: #113410. |
https://bugs.python.org/issue37501