nedbat / coveragepy

The code coverage tool for Python
https://coverage.readthedocs.io
Apache License 2.0
3.01k stars 432 forks source link

Branch coverage in finally block unexpectedly affected by catch block #1456

Open vsajip opened 2 years ago

vsajip commented 2 years ago

Describe the bug If a block such as

try:
   # some code
finally:
  # some finally code

has 100% branch coverage, then adding an exception block can change things even when the exception isn't triggered:

try:
     # some code
except SomeException as e:  # pragma: no cover
    raise SomeOtherException() from e
finally:
    # some finally code

The finally code branch coverage goes down.

To Reproduce How can we reproduce the problem? Please be specific. Don't link to a failing CI job. Answer the questions below:

  1. What version of Python are you using? See coverage debug sys output below.
  2. What version of coverage.py shows the problem? The output of coverage debug sys is helpful. See coverage debug sys output below.
  3. What versions of what packages do you have installed? The output of pip freeze is helpful. Nothing relevant, I run coverage from a venv and my code under test has no dependencies.
  4. What code shows the problem? Give us a specific commit of a specific repo that we can check out. If you've already worked around the problem, please provide a commit before that fix. https://github.com/vsajip/pagesign/blob/ac5c6c1096763b1f858efbdf7385564f394246b9/pagesign.py
  5. What commands did you run? coverage run --branch test_pagesign.py
$ coverage debug sys
-- sys -------------------------------------------------------
               coverage_version: 6.4.4
                coverage_module: /home/vinay/.local/share/virtualenvs/coverage/lib/python3.7/site-packages/coverage/__init__.py
                         tracer: -none-
                        CTracer: available
           plugins.file_tracers: -none-
            plugins.configurers: -none-
      plugins.context_switchers: -none-
              configs_attempted: .coveragerc
                                 setup.cfg
                                 tox.ini
                                 pyproject.toml
                   configs_read: /home/vinay/projects/pagesign/setup.cfg
                    config_file: None
                config_contents: -none-
                      data_file: -none-
                         python: 3.7.3+ (heads/3.7:0ff08b061b, May 11 2019, 09:27:05) [GCC 7.4.0]
                       platform: Linux-4.15.0-193-generic-x86_64-with-debian-buster-sid
                 implementation: CPython
                     executable: /home/vinay/.local/share/virtualenvs/coverage/bin/python3
                   def_encoding: utf-8
                    fs_encoding: utf-8
                            pid: 51020
                            cwd: /home/vinay/projects/pagesign
                           path: /disk2/vinay/.local/share/virtualenvs/coverage/bin
                                 /home/vinay/.local/share/virtualenvs/coverage/lib/python37.zip
                                 /home/vinay/.local/share/virtualenvs/coverage/lib/python3.7
                                 /home/vinay/.local/share/virtualenvs/coverage/lib/python3.7/lib-dynload
                                 /home/vinay/.local/lib/python3.7
                                 /home/vinay/.local/share/virtualenvs/coverage/lib/python3.7/site-packages
                    environment: HOME = /home/vinay
                   command_line: /home/vinay/.local/share/virtualenvs/coverage/bin/coverage debug sys
                sqlite3_version: 2.6.0
         sqlite3_sqlite_version: 3.22.0
             sqlite3_temp_store: 0
        sqlite3_compile_options: COMPILER=gcc-7.5.0, ENABLE_COLUMN_METADATA, ENABLE_DBSTAT_VTAB,
                                 ENABLE_FTS3, ENABLE_FTS3_PARENTHESIS, ENABLE_FTS3_TOKENIZER, ENABLE_FTS4,
                                 ENABLE_FTS5, ENABLE_JSON1, ENABLE_LOAD_EXTENSION, ENABLE_PREUPDATE_HOOK,
                                 ENABLE_RTREE, ENABLE_SESSION, ENABLE_STMTVTAB, ENABLE_UNLOCK_NOTIFY,
                                 ENABLE_UPDATE_DELETE_LIMIT, HAVE_ISNAN, LIKE_DOESNT_MATCH_BLOBS,
                                 MAX_SCHEMA_RETRY=25, MAX_VARIABLE_NUMBER=250000, OMIT_LOOKASIDE,
                                 SECURE_DELETE, SOUNDEX, TEMP_STORE=1, THREADSAFE=1

Expected behavior I didn't expect to see partial branch coverage for the finally block when adding an except block that isn't triggered.

Additional context See source lines 262 and 520 in https://app.codecov.io/gh/vsajip/pagesign/commit/ac5c6c1096763b1f858efbdf7385564f394246b9/pagesign.py - though sometimes codecov.io can't display the page. When testing locally, the annotation for a block such as

L01 try:
L02     # some code
L03 except SomeException as e:  # pragma: no cover
L04     raise SomeOtherException() from e
L05 finally:
L06     # some finally code

says L06 ↛ exit and with a tooltip that says line L06 didn't except from function '<funcname>', because the raise on line L04 wasn't executed. This is confusing because I wouldn't have expected L06 to "except from function" - not sure what's meant there.

Can I add or change pragmas to avoid this decrease in coverage?

vsajip commented 2 years ago

Short reproducing program:

import os
import subprocess
import tempfile

class CustomException(Exception): pass

def run_command(cmd, wd):
    p = subprocess.Popen(cmd, cwd=wd,
                         stdout=subprocess.PIPE, stderr=subprocess.PIPE)
    stdout, stderr = p.communicate()
    if p.returncode == 0:
        return stdout, stderr
    else:  # pragma: no cover
        raise subprocess.CalledProcessError(p.returncode,
                                            p.args,
                                            output=stdout,
                                            stderr=stderr)

def main():
    fd, fn = tempfile.mkstemp(prefix='test-')
    os.write(fd, b'Hello, world!')
    os.close(fd)
    try:
        run_command(['ls', '-l', fn], os.getcwd())
    except subprocess.CalledProcessError as e:  # pragma: no cover
        raise CustomException('Child process failed') from e
    finally:
        os.remove(fn)
    return 0

if __name__ == '__main__':  # pragma: no branch
    main()
nedbat commented 2 years ago

I think I understand what is happening here. In your first code example:

try:
   # some code
finally:
  # some finally code

the "finally" code can only execute one way: because "some code" has finished. There's only one path through the finally code, and it is executed, giving you 100% branch coverage.

In the second code:

try:
     # some code
except SomeException as e:  # pragma: no cover
    raise SomeOtherException() from e
finally:
    # some finally code

there are two ways to run "some finally code": "some code" can finish normally (which it does), and "raise SomeOtherException" can happen (which it doesn't). There are two paths through the finally code, and only one of them happens. This gives you 50% branch coverage.

The complicating factor here is the pragma noting that the raise can never happen. Coverage.py isn't taking that into account in deciding on the branch coverage in the finally clause. I'm not sure what it would take for it to understand that, but it's an interesting case.

In the meantime, you can add # pragma: no branch to the line with 50% branch coverage to indicate that you know it is only partial.

vsajip commented 2 years ago

OK, thanks for the explanation. Shall I close the issue? I ask in case you want to look into it further. I added some test cases to make sure the exception was raised, and now I have 100% coverage again.

Thank you so much for all your work on coverage.