gh-69113: Fix doctest to report line numbers for __test__ strings #141624
Conversation
Lib/doctest.py
Outdated
| # contains any unique line. | ||
| for offset, line in enumerate(obj.splitlines(keepends=True)): | ||
| if source_lines.count(line) == 1: | ||
| return source_lines.index(line) - offset |
There was a problem hiding this comment.
If I'm understanding this correctly, it would be more precise to say "at least one line that is unique in the file". It also still has the limitation that it finds them only if they are in triple quoted string format (as opposed to for example the admittedly unlikely form of '>>>test\nresult\n')
My version would find a test even it there wasn't a unique line, but is (probably?) less efficient. I'm not sure how likely it is for the "no unique line" case to happen, but with multiple doctests I have the feeling the answer is not 0.
Thoughts?
There was a problem hiding this comment.
You're right - this approach requires at least one line unique in the entire file. Your multi-line matching approach is more robust since it only needs the sequence of lines to be unique, not any individual line. If >>> x = 1 appears multiple times but the subsequent lines differ, yours would still find the correct location, while this one would fail. The current implementation falls back to None gracefully in that case, but your approach would handle more real-world scenarios.
Would you prefer we use your multi-line matching instead, or perhaps a hybrid that tries the simple approach first and falls back to multi-line matching?
There was a problem hiding this comment.
The no unique line cases can be low, but we can't treat them as zero, so I'm leaning towards a hybrid approach or building upon your implementation to cover edge cases.
There was a problem hiding this comment.
Yeah, a hybrid approach sounds good.
There was a problem hiding this comment.
@bitdancer can you please take another look here?
CuriousLearner
force-pushed
the
fix-gh-69113
branch
2 times, most recently
from
4b6628a to
55e987b
Compare
bitdancer
left a comment
bitdancer
left a comment
There was a problem hiding this comment.
Looks good to me. Just some style comments.
Lib/doctest.py
Outdated
| # Skip the first line (may be on same line as opening quotes) | ||
| # and any blank lines to find a meaningful line to match | ||
| start_index = 1 | ||
| while start_index < len(obj_lines) and not obj_lines[start_index].strip(): |
There was a problem hiding this comment.
Lines should be wrapped <80.
I also don't find the extra blank lines around the 'skip' block helpful, but I'm not going to object if you leave them.
| self.assertEqual(len(include_empty_finder.find(mod)), 1) | ||
| self.assertEqual(len(exclude_empty_finder.find(mod)), 0) | ||
|
|
||
| def test_lineno_of_test_dict_strings(self): |
There was a problem hiding this comment.
As above, lines should be wrapped < 80. The existing code is mostly consistent about that outside of the places where the doctest text has to be unwrapped.
|
|
||
| sys.path.insert(0, tmpdir) | ||
| try: | ||
| # Import the module |
There was a problem hiding this comment.
Most of these comments just restate the code and so don't really add anything. Comments like that tend to rot faster than the code.
|
|
||
| # Assert that we found the test and it has a valid line number | ||
| self.assertIsNotNone(test_dict_test, "__test__ dict test not found") | ||
| # The line number should not be None - this is what the bug fix addresses |
There was a problem hiding this comment.
This comment, however, is helpful ;) You could also mention the bug number here.
| self.assertIsNotNone(test_dict_test, "__test__ dict test not found") | ||
| # The line number should not be None - this is what the bug fix addresses | ||
| self.assertIsNotNone(test_dict_test.lineno, | ||
| "Line number should not be None for __test__ dict strings") |
There was a problem hiding this comment.
I think modern style is to put a carriage return after the ( and indent all the arguments four spaces from the start of the line, one argument per line.
|
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 |
Enhanced the _find_lineno method in doctest to correctly identify and report line numbers for doctests defined in __test__ dictionaries when formatted as triple-quoted strings. Finds a non-blank line in the test string and matches it in the source file, verifying subsequent lines also match to handle duplicate lines. Previously, doctest would report "line None" for __test__ dictionary strings, making it difficult to debug failing tests. Co-Authored-By: Jurjen N.E. Bos <[email protected]> Co-Authored-By: R. David Murray <[email protected]>
CuriousLearner
force-pushed
the
fix-gh-69113
branch
from
55e987b to
5dd004a
Compare
2 participants
Enhanced the
_find_linenomethod in doctest to correctly identify and report line numbers for doctests defined in__test__dictionaries when formatted as triple-quoted strings. The implementation finds unique lines in the test string and matches them in the source file to calculate the correct starting line number.Previously, doctest would report
"line None"for__test__dictionary strings, making it difficult to debug failing tests.Fixes #69113