bpo-31908: Fix output of cover files for trace module command-line tool #4205
Conversation
Rather than nesting an if/else inside the last else of the outer condition, the conditional can be flattened into four possibilities.
A blank line will not appear in ``lnotab`` as returned by ``_find_executable_linenos``. Alternately, ``lnotab`` might be set to an empty dict. Either way, the check for whether a line is blank can be done after checking for whether the ``lineno`` is in ``lnotab``.
There's no need to check for an empty line. Either we hit the line and wrote the count, we didn't hit the line and wrote an annotation, or we want to print some spaces to indent the line.
The lnotab parameter shouldn't be renamed, for compatibility, but it could use some further explanation. One natural reading of it would be "lines without tabs", but it's really a "line number table" -- a mapping of line numbers to possible executable lines. One nuance to that "table" is that ``_find_executable_linenos`` will ignore some code that the user might want to highlight as unreachable. For example, code inside a condition like ``if False:`` will be excluded. Further, it appears ``lnotab`` doesn't need to be a dict. A set would suffice and might reduce the memory requirements of the trace module. If the ``_find_lines`` function is used only by the trace module, as the underscore prefix suggests, it could be refactored to return a set of line numbers instead of a mapping of line numbers to lines.
Whether the show_missing parameter is true or false, the cover file should be written. The ``lnotab`` parameter will be used simply to decide whether to annotate unreached lines with some extra characters.
serhiy-storchaka
added
type-bug
serhiy-storchaka
changed the title
|
This is waiting on me (or someone) to write some tests to help prevent future regressions. |
The single test function checks that the coverfile is correctly written and contains appropriate line count and unreached code highlighting. The check for ``if []`` avoids interpreter optimization which might allow the interpreter to recognize the conditional statement as a no-op and eliminate it. For example, ``if False`` would not be counted, nor would any code in the following block be highlighted by the trace.
|
@berkerpeksag I added a test case, treating the CLI as a black box. |
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
The test is passed if revert changes in trace.py. Please ensure that it fails with unpatched code.
Lib/test/test_trace.py
Outdated
| def setUp(self): | ||
| with open(self.codefile, 'w') as f: | ||
| f.write(textwrap.dedent('''\ | ||
| x = 42 |
There was a problem hiding this comment.
Please increase the indentation of these lines for readability.
The --missing or -m option was hiding the bug. Testing the CLI with --count option and only that option reveals the bug.
|
Whoops. I forgot that the Also, indented the triple-quote. |
|
Tests are failed because It may by simpler to not use self.assertEqual(f.read(), '''\
1: x = 42
1: if []:
print('unreachable')
''')or self.assertEqual(f.read(),
" 1: x = 42\n"
" 1: if []:\n"
" print('unreachable')\n"
) |
|
I chose the 2nd of those two suggestions. While the explicit newlines are awkward, I think the mismatched indentation of the closing brace was the worse evil. My apologies for not checking the test pass/fail. It correctly passes the tests now in the branch. |
|
Thanks @selik for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7. |
…ol. (pythonGH-4205) Previously emitted cover files only when --missing option was used. (cherry picked from commit 47ab154) Co-authored-by: Michael Selik <[email protected]>
|
GH-6666 is a backport of this pull request to the 3.7 branch. |
…ol. (pythonGH-4205) Previously emitted cover files only when --missing option was used. (cherry picked from commit 47ab154) Co-authored-by: Michael Selik <[email protected]>
|
GH-6667 is a backport of this pull request to the 3.6 branch. |
…ol. (GH-4205) Previously emitted cover files only when --missing option was used. (cherry picked from commit 47ab154) Co-authored-by: Michael Selik <[email protected]>
|
Thanks @selik for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6. |
…ol. (pythonGH-4205) Previously emitted cover files only when --missing option was used. (cherry picked from commit 47ab154) Co-authored-by: Michael Selik <[email protected]>
|
GH-6668 is a backport of this pull request to the 3.6 branch. |
…ol. (GH-4205) Previously emitted cover files only when --missing option was used. (cherry picked from commit 47ab154) Co-authored-by: Michael Selik <[email protected]>
5 participants
The
lnotabvariable -- a dict of executable lines -- is a helper for theshow_missingparameter. Whenshow_missingis false, we don't need to calculate that dict. However, emitting the cover files had been made conditional on that dict being non-empty in Commit f026dae. This reverts part of the commit to ensure the cover files are emitted regardless of theshow_missingargument.Additionally, the
write_results_fileis refactored to clarify the purpose oflnotab.https://bugs.python.org/issue31908