Coverage ignores implicit else in try blocks

[HalfWhitt]

Describe the bug

I know I already mentioned this in #3569 and #3589, but I think it should have its own place.

I've discovered something I find unnerving... If you have a try / except with no else, Coverage doesn't make sure you've tested it without catching the exception.

nedbat/coveragepy#877

Steps to reproduce

Consider the following code, from the issue linked above:

def f(x):
    try:
        y = 1/x
    except ZeroDivisionError:
        y = 0
    return y

One would presumably like to test at least two cases: one in which the ZeroDivisionError is encountered, and one in which it isn't. However, if you only test f(0), Coverage will tell you there are no missed branches. In order for Coverage to consider the no-error case, you need an explicit else:

def f(x):
    try:
        y = 1/x
    except ZeroDivisionError:
        y = 0
    else:
        pass
    return y

Expected behavior

Ideally, we would have a way to make sure we're exercising both options in a try / except — or potentially more than two, if the except catches more than one type of error.

Environment

Coverage 7.9.1

Additional context

I've not attempted anything close to an exhaustive search, but there are definitely try blocks in Toga's code that have no else. We might be testing both the exception and no-exception cases, but currently we have no way of systematically ensuring this.

[freakboy3742]

Interesting... definitely worth an investigation to see if we've missed anything notable. Adding empty else blocks to every try:except: and seeing if we get any coverage misses should be a reasonable proxy for this.

[HalfWhitt]

I should probably resist the urge to start looking at Coverage's source...

[freakboy3742]

No... let the yak shaving urge overtake your soul... 😜

[mhsmith]

This isn't quite as bad as it sounds, as it only affects cases where an exception is always thrown by the last line in the try block. If the exception was always thrown earlier, then there would be uncovered lines below it.

[HalfWhitt]

This isn't quite as bad as it sounds, as it only affects cases where an exception is always thrown by the last line in the try block. If the exception was always thrown earlier, then there would be uncovered lines below it.

That's true... but a relatively naïve regex search turns up at least a hundred spots in the code base with only a single line inside a try (although it would take more looking, or a much more complicated regex, to see how many of them lack an else). It is, after all, generally good practice to put as little as possible inside the try aside from what you expect could cause the exception.

[HalfWhitt]

Note: while I was aware Coverage was blind to ternary if/then/else expressions, it hadn't occurred to me until seeing it mentioned that the same applies to any compound boolean checks. if a or b or c potentially has eight different test cases, but Coverage sees only two.

[johnzhou721]

For the specific try-except-else case, I've gotten ChatGPT to provide a script to do the replacement:

#!/usr/bin/env python3
"""
Script to add `else: pass` to every try-except block that lacks an else clause
in all Python files in the current directory (recursively).

Usage:
    python add_else_pass.py

Backups of modified files will be created with the `.bak` extension.
"""
import ast
import astor
import os
import shutil

class TryElseTransformer(ast.NodeTransformer):
    def visit_Try(self, node: ast.Try) -> ast.Try:
        # Only add else if there are except handlers and no existing else
        if node.handlers and not node.orelse:
            # Create a Pass statement
            pass_node = ast.Pass()
            # Add pass under else
            node.orelse = [pass_node]
        # Continue transforming nested nodes
        self.generic_visit(node)
        return node


def process_file(path: str):
    with open(path, 'r', encoding='utf-8') as f:
        source = f.read()

    try:
        tree = ast.parse(source, filename=path)
    except SyntaxError as e:
        print(f"Skipping {path}: SyntaxError - {e}")
        return

    transformer = TryElseTransformer()
    new_tree = transformer.visit(tree)
    ast.fix_missing_locations(new_tree)

    new_source = astor.to_source(new_tree)

    if new_source != source:
        # Backup original
        bak_path = path + '.bak'
        shutil.copy2(path, bak_path)
        # Write modified
        with open(path, 'w', encoding='utf-8') as f:
            f.write(new_source)
        print(f"Updated {path}, backup at {bak_path}")


def main():
    # Walk current directory
    for root, dirs, files in os.walk('./core/src/toga'):
        # Skip hidden directories
        dirs[:] = [d for d in dirs if not d.startswith('.')]
        for file in files:
            if file.endswith('.py'):
                full_path = os.path.join(root, file)
                process_file(full_path)

if __name__ == '__main__':
    main()

Result is at johnzhou721/testelse. This removes a bunch of comments and reformats the code because it parses everything and then rewrites the scripts.

Running tox and matching up the line numbers and excluding the no-cover comments removed, it seems like the cases with no exceptions in

toga/core/src/toga/style/pack.py

Lines 137 to 160 in b7c1e92

	def __set__(self, obj, value):
	if value is self:
	# This happens during autogenerated dataclass __init__ when no value is
	# supplied.
	return
	self.warn_if_deprecated()
	# Delete the other property when setting this one.
	try:
	delattr(obj, f"_{self.other}")
	except AttributeError:
	pass
	super().__set__(obj, value)
	def __delete__(self, obj):
	self.warn_if_deprecated()
	# Delete the other property too.
	try:
	delattr(obj, f"_{self.other}")
	except AttributeError:
	pass
	super().__delete__(obj)

are not tested.

Nevertheless when I ruff the output of that script I get a bunch less coverage... even files with no coverage. No idea why, so this is just a ruff estimate of the impact (pun intended).

And indeed, this issue is real. with this diff off of latest main

diff --git a/core/src/toga/style/pack.py b/core/src/toga/style/pack.py
index f3ab4575f..beadd97ec 100644
--- a/core/src/toga/style/pack.py
+++ b/core/src/toga/style/pack.py
@@ -147,6 +147,8 @@ class _alignment_property(validated_property):
             delattr(obj, f"_{self.other}")
         except AttributeError:
             pass
+        else:
+            pass
         super().__set__(obj, value)
 
     def __delete__(self, obj):
@@ -157,6 +159,8 @@ class _alignment_property(validated_property):
             delattr(obj, f"_{self.other}")
         except AttributeError:
             pass
+        else:
+            pass
         super().__delete__(obj)
 
     def is_set_on(self, obj):

We get

Name                     Stmts   Miss Branch BrPart  Cover   Missing
--------------------------------------------------------------------
src/toga/style/pack.py     388      2    164      0  99.6%   151, 163
--------------------------------------------------------------------
TOTAL                     5520      2   1266      0  99.9%

[johnzhou721]

TL;DR I've caught at least 1 issue.

[HalfWhitt]

At least that particular case is only in a backwards compatibility shim... nevertheless, it's not ideal, and it suggests there could be other relevant places too.

[johnzhou721]

@HalfWhitt Hmm... Running ruff over the output of that script seems to bring some stuff down to like 0 coverage... I haven't investigated yet, but it is possible that ruff is making some errors.

Anyways to start actually understanding the scale of the problem we'd need to have a better way to place these branches than that ChatGPT-thrown-together script. There could be more places, I agree. Also comments above which I just skimmed suggests other branches not just this case I replaced, so this estimate is really ruff :)

[HalfWhitt]

Honestly, before I started peppering our codebase with a hundred else: pass lines, I'd first investigate contributing to Coverage. Unlike it inability to see inline branches (ternary/and/or), its inability to see implicit elses isn't a structural issue baked into the core assumptions of the program; that is, it should theoretically be possible to fix without a major refactor. Is that feasible? Maybe, maybe not. But I think it's worth a look.