Turn on branch coverage checking. #887

freakboy3742 · 2022-09-29T23:05:42Z

I wasn't aware that (a) coverage had an option for branch coverage, and (b) that it wasn't turned on by default.

This turns on branch coverage, and moves the coverage configuration to pyproject.toml. This reveals a handful of edge cases that aren't currently covered.

This also addresses the noisy emails caused by CodeCov... by removing codecov.

Fixes #848.

PR Checklist:

All new features have been tested
All new features have been documented
I have read the CONTRIBUTING.md file
I will abide by the code of conduct

freakboy3742 · 2022-09-30T05:27:00Z

@mhsmith @rmartin16 This is still in draft form, but I've got it producing a coverage report as an alternative to CodeCov. We'll likely need to get to 100% coverage for this report to be useful; we'd be losing the ability to report on "coverage of this PR", and the ability to ratchet the minimum allowed coverage as tests are added.

I'd be interested in your thoughts on how this output compares.

rmartin16 · 2022-09-30T15:23:06Z

I'm mostly indifferent regarding exactly how coverage is tracked....but I don't think I'm convinced removing CodeCov creates a better situation necessarily. I think the benefits outweighs the negatives....and we can probably address the negatives.

CodeCov notification noise
Given this PR does all the work to combine the coverage reports, couldn't we just upload that single combined report to CodeCov?

Even if we cant, turning off the CodeCov PR comments still leaves the CodeCov check that contains all the same information and is as accessible from details link in the list of checks.

"Coverage decrease" false positives
CodeCov supports a threshold parameter to help avoid this....albeit in a codecov.yml file.

Final coverage report
While I really like the native html report from coverage, this strategy of providing it as zip file is particularly clunky.
If we keep this strategy:

It might be better to target something like a MHTML file where we're just dealing with a single file to show the report. (although, Ive done zero legwork to determine feasibility...)
Ensure a top-level directory in the zip. This avoids potentially dumping all these files in to whatever directory it's in.

freakboy3742 · 2022-10-01T00:04:45Z

Uploading a single combined report is a good thought - I hadn't considered that, but it would seem to address the biggest complaint described by #848.

I'm not sure the threshold approach fixes the problem; the issue is with the algorithm used to compute coverage. If a file has 100 lines and 95% coverage, deleting 10 lines drops coverage to ~85%. The only way to address this is to fix this is to have 100% coverage to start with. The good news is that from looking at the current coverage misses, we're actually not that far from getting 100% coverage. The missing cases are mostly things like "checking that the Windows filename actually has .bat", or "actually running validate_tools() on a build step" - things that are easy to add tests for, if we have a focussed effort.

Looking at the HTML coverage report - unfortunately, there aren't a whole lot of options there. coverage html doesn't provide any customisation options (that I'm aware of) that would provide MHTML output or include a directory. Those all sound like good suggestions, but they'd be upstream contributions to Coverage (and almost certainly substantial projects in themselves).

That said - I'm actually wondering whether the HTML report is valuable at all. The CI failure includes a summary report which details the specific lines that are missing; those lines are easy enough to find locally in your editor. The printed code context is pretty... but I'm not sure it's necessarily helpful. If we can get to 100% coverage as a baseline, then a new PR that misses coverage will only be reporting lines that are a problem in that PR.

rmartin16 · 2022-10-01T02:54:37Z

the issue is with the algorithm used to compute coverage

Yeah; that makes sense. Although, I think this issue is moot if we need to get to 100% coverage either way.

Another feature of CodeCov I really like is the lines without coverage are called out directly in the github diff. Although, that information is just as available from the coverage report.

pyproject.toml

freakboy3742 · 2022-10-02T09:23:48Z

src/briefcase/__main__.py

@@ -16,7 +17,7 @@ def main():
        command = Command(logger=logger, console=console)
        options = command.parse_options(extra=extra_cmdline)
        command.check_obsolete_data_dir()
-        command.parse_config("pyproject.toml")
+        command.parse_config(Path.cwd() / "pyproject.toml")


This makes the path absolute, which means it can be tested reliably by monkeypatching cwd.

freakboy3742 · 2022-10-02T09:24:19Z

src/briefcase/__main__.py

@@ -41,8 +42,8 @@ def main():
        with suppress(KeyboardInterrupt):
            logger.save_log_to_file(command)

-    sys.exit(result)
+    return result


This makes it easier to test the result, as you can test the return value, rather than catching the sys.exit()

freakboy3742 · 2022-10-02T09:24:59Z

src/briefcase/commands/base.py

@@ -117,7 +117,7 @@ def __init__(
        console: Console,
        tools: ToolCache = None,
        apps: dict = None,
-        base_path: Path = Path.cwd(),
+        base_path: Path = None,


This ensures that Path.cwd() is evaluated when the instance is instantiated, rather than when the code is imported, which means it's testable for the default case.

freakboy3742 · 2022-10-02T09:25:54Z

src/briefcase/commands/base.py

-                )
-        except IndexError as e:
+
+        if len(app_home) == 0:


This was a bug; previously assumed to be caught as an IndexError, it was being returned as a BriefcaseCommandError with the wrong message.

freakboy3742 · 2022-10-02T09:26:46Z

src/briefcase/commands/create.py

-        if app_path.is_dir():
-            self.tools.shutil.rmtree(app_path)
-            self.tools.os.mkdir(app_path)
+        self.tools.shutil.rmtree(app_path)


The if is_dir wasn't needed, as the app_path is always a directory, generated by the template.

freakboy3742 · 2022-10-02T09:29:58Z

src/briefcase/commands/publish.py

-        self.logger.info(f"TODO: Publish {app.app_name} to {channel}")
+        self.logger.info(
+            f"TODO: Publish {app.app_name} to {channel}"
+        )  # pragma: no cover


This is effectively a raise NotImplementedError(), but with nicer optics if it's invoked.

freakboy3742 · 2022-10-02T09:30:50Z

src/briefcase/platforms/macOS/app.py

-                    os.fsdecode(Path(tmpdir) / "lib"),
-                    os.fsdecode(lib_path),
-                )
-


This was only needed to support old-style support packages; now that we're version locked on support packages, this isn't needed.

freakboy3742 · 2022-10-02T09:32:54Z

Down to just 5 uncovered branches - but it's not obvious what those 5 are even describing...

mhsmith · 2022-10-02T16:31:58Z

Down to just 5 uncovered branches - but it's not obvious what those 5 are even describing...

src/briefcase/console.py                      202      0     68      1    99.6%   166->170

This means the tests don't cover the case where the if expression is false and the if block is skipped.

src/briefcase/integrations/android_sdk.py     448      0    161      1    99.8%   298->264

As above, but in this case the if block is the last statement in a with block, so if the expression is false then the next code to execute will be the exit method of the context manager.

src/briefcase/integrations/subprocess.py      225      0     87      1    99.7%   496->490

As above, but in this case the if block is the last statement in a while block, so if the expression is false then the next code to execute will be the while expression.

src/briefcase/platforms/macOS/__init__.py     222      0     88      2    99.4%   107->exit, 468->exit

Not sure about these ones, but based on the discussion here it might have something to do with the code throwing an exception and therefore never completing the function normally.

freakboy3742 · 2022-10-02T23:55:27Z

Down to just 5 uncovered branches - but it's not obvious what those 5 are even describing...
src/briefcase/console.py                      202      0     68      1    99.6%   166->170
This means the tests don't cover the case where the if expression is false and the if block is skipped.

🤦 That one was obvious when I took a second look. I was missing the "prefix" requirement for that for that line to be (not) executed.

src/briefcase/integrations/android_sdk.py     448      0    161      1    99.8%   298->264
As above, but in this case the if block is the last statement in a with block, so if the expression is false then the next code to execute will be the exit method of the context manager.

That's what I thought too... but we run the test suite on Windows, which should (and does, based on my testing) trigger that branch. Looking closer, I'm wondering if it might be a problem with the merging of the coverage data.

src/briefcase/integrations/subprocess.py      225      0     87      1    99.7%   496->490
As above, but in this case the if block is the last statement in a while block, so if the expression is false then the next code to execute will be the while expression.

Ah - now I see what was tripping me up. It's a case that can't actually happen (readline() returning None).

src/briefcase/platforms/macOS/__init__.py     222      0     88      2    99.4%   107->exit, 468->exit
Not sure about these ones, but based on the discussion here it might have something to do with the code throwing an exception and therefore never completing the function normally.

Hrm... that's as good a theory as any, I suppose... I'll keep poking.

freakboy3742 · 2022-10-03T00:02:21Z

src/briefcase/integrations/subprocess.py

@@ -493,7 +493,7 @@ def _stream_output_thread(self, popen_process):
                output_line = ensure_str(popen_process.stdout.readline())
                if output_line:
                    self.tools.logger.info(output_line)
-                elif output_line == "":
+                else:


The old version of this left an open coverage case for readline() returning None. This shouldn't ever happen; if it does, it's probably safe to assume that the process has ended, so I've made this a bucket else case.

freakboy3742 · 2022-10-03T00:54:03Z

This is really weird - it looks like there's a discrepancy between 3.9 and 3.10 in how they report exiting a context manager.

Looking at the missing branch on android_sdk.py:

Python 3.9 on Windows reports coverage of that branch, only missing 299-301 (as expected)
Python 3.9 on macOS reports 298-304 as the missing branch
Python 3.9 on ubuntu reports 298-304 as the missing branch
Python 3.10 on ubuntu reports 298-264 as the missing branch

I'm guessing that since we're not reporting 3.10 on Windows, there's no match for the 298-264 missing branch, which is then reported as a miss in the overall results.

rmartin16 · 2022-10-03T03:07:56Z

💯

codecov · 2022-10-03T03:28:50Z

Codecov Report

Merging #887 (fb4a4b3) into main (24f4b5c) will decrease coverage by 1.42%.
The diff coverage is n/a.

❗ Current head fb4a4b3 differs from pull request most recent head b3dc611. Consider uploading reports for the commit b3dc611 to get more accurate results

Impacted Files	Coverage Δ
src/briefcase/commands/package.py	`79.06% <0.00%> (-6.98%)`	⬇️
src/briefcase/integrations/xcode.py	`94.33% <0.00%> (-2.84%)`	⬇️
src/briefcase/platforms/macOS/app.py	`98.03% <0.00%> (-1.97%)`	⬇️
src/briefcase/platforms/macOS/xcode.py	`96.15% <0.00%> (-1.93%)`	⬇️
src/briefcase/integrations/subprocess.py	`98.66% <0.00%> (-1.34%)`	⬇️
src/briefcase/console.py	`99.50% <0.00%> (-0.50%)`	⬇️
src/briefcase/commands/run.py	`94.11% <0.00%> (-0.48%)`	⬇️
src/briefcase/integrations/android_sdk.py	`98.88% <0.00%> (-0.45%)`	⬇️
src/briefcase/commands/create.py	`99.40% <0.00%> (-0.30%)`	⬇️
src/briefcase/integrations/linuxdeploy.py	`100.00% <0.00%> (ø)`
... and 2 more

freakboy3742 · 2022-10-03T03:36:38Z

Now that we're at 100% coverage, the only question that remains is whether we stick with "self-hosted" coverage testing, or revert to CodeCov reporting.

As I see it, CodeCov is an external dependency which has been flaky from time to time, and I'm always in favour of having less external dependencies if we can. It's also very noisy, although only uploading a single CI report may mitigate that somewhat. The major advantage is that it provides an in-browser way to view coverage results.

Self-hosted coverage isn't as pretty, but it does present the specific lines in the test failure, rather than needing a couple of clicks to get there. It also matches the report you'll get if you run coverage locally. We can generate HTML reports, but they're not easy to browse (you have to download to view them).

On that basis, I have a mild preference to drop CodeCov and go self-hosted, dropping the upload of the coverage HTML result. However, if anyone has a strong preference to retain CodeCov, I won't put up much of a fight.

Opinions?

saroad2 · 2022-10-03T08:37:40Z

The only benefit of CodeCov was the fact that it helped us make sure that every PR is keeping the total coverage rate high enough and that every new change is covered by tests.

Now that we have finally got to 100% coverage, this feature is no longer needed. Simply add the directive fail_under = 100 and now every new line of code must be tested. In that sense, self-hosted coverage is perfect, no need for any external tools.

The "downsize" of this is that now we will have to enforce 100% coverage all the time, without exceptions (or else, the CI process will not pass). In my opinion, it's a feature, not a bug :)

freakboy3742 added 6 commits September 30, 2022 07:03

Turn on branch coverage checking.

df730a4

Add changenote.

f8d24f5

Produce a local code coverage report.

fb4a4b3

Remove Codecov.

a80b825

Modify test invocation to get mergeable artefacts.

0cd2ca0

Debug help for generating coverage.

da2a1ae

freakboy3742 force-pushed the branch-coverage branch 3 times, most recently from 70654ea to cf31773 Compare September 30, 2022 03:53

Use relative paths for coverage, and correct windows paths.

7ae7b63

freakboy3742 force-pushed the branch-coverage branch from cf31773 to 7ae7b63 Compare September 30, 2022 05:28

freakboy3742 added 3 commits October 1, 2022 09:50

Removed some unused declarations that caused problems for coverage.

8302fed

Removed some legacy support package handling code.

981e5ff

First pass at increasing coverage.

5ada7d1

freakboy3742 force-pushed the branch-coverage branch from 6233318 to 5ada7d1 Compare October 1, 2022 02:45

freakboy3742 added 5 commits October 2, 2022 09:13

Correct logic when unable to find app module code.

f26a1bb

Correct logic around cleaning up the app folder.

f8528e8

Add coverage for commands.

ef1ae24

Add tests for the mainline.

fd02607

Almost at 100%...

ed9ab08

freakboy3742 commented Oct 2, 2022

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

freakboy3742 commented Oct 2, 2022

View reviewed changes

nedbat mentioned this pull request Oct 2, 2022

Use forward slashes in relative paths on Windows nedbat/coveragepy#991

Closed

Getting close...

b9d3870

freakboy3742 force-pushed the branch-coverage branch from c35b6b7 to 5f86e9d Compare October 3, 2022 00:00

freakboy3742 commented Oct 3, 2022

View reviewed changes

Add debug for coverage merging

7fa5a9a

freakboy3742 force-pushed the branch-coverage branch from 5f86e9d to 7fa5a9a Compare October 3, 2022 00:24

freakboy3742 force-pushed the branch-coverage branch from 5c1a8dc to 0a2b483 Compare October 3, 2022 01:02

Merge smoke, platform, and version tests into a single test matrix.

0c7b5e2

freakboy3742 force-pushed the branch-coverage branch from 0a2b483 to 0c7b5e2 Compare October 3, 2022 01:08

freakboy3742 added 2 commits October 3, 2022 10:37

Add extra branch tests for notarization.

9aec45f

100% coverage!

e9fbdf7

freakboy3742 added 3 commits October 3, 2022 11:17

Small cleanup of CI steps.

a531a71

Coverage without any omissions.

7e1f8ed

Bumped the checkout plugin version.

b3dc611

freakboy3742 marked this pull request as ready for review October 3, 2022 07:16

freakboy3742 requested a review from mhsmith October 3, 2022 07:16

mhsmith approved these changes Oct 3, 2022

View reviewed changes

mhsmith merged commit a6a59fa into beeware:main Oct 3, 2022

freakboy3742 deleted the branch-coverage branch October 3, 2022 22:05

saroad2 mentioned this pull request Oct 5, 2022

Let's reach that 100% coverage! beeware/toga#1601

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn on branch coverage checking. #887

Turn on branch coverage checking. #887

freakboy3742 commented Sep 29, 2022 •

edited

freakboy3742 commented Sep 30, 2022

rmartin16 commented Sep 30, 2022 •

edited

freakboy3742 commented Oct 1, 2022

rmartin16 commented Oct 1, 2022

freakboy3742 Oct 2, 2022

freakboy3742 Oct 2, 2022

freakboy3742 Oct 2, 2022

freakboy3742 Oct 2, 2022

freakboy3742 Oct 2, 2022

freakboy3742 Oct 2, 2022

freakboy3742 Oct 2, 2022

freakboy3742 commented Oct 2, 2022

mhsmith commented Oct 2, 2022

freakboy3742 commented Oct 2, 2022

freakboy3742 Oct 3, 2022

freakboy3742 commented Oct 3, 2022

rmartin16 commented Oct 3, 2022

codecov bot commented Oct 3, 2022

freakboy3742 commented Oct 3, 2022

saroad2 commented Oct 3, 2022

Turn on branch coverage checking. #887

Turn on branch coverage checking. #887

Conversation

freakboy3742 commented Sep 29, 2022 • edited

PR Checklist:

freakboy3742 commented Sep 30, 2022

rmartin16 commented Sep 30, 2022 • edited

freakboy3742 commented Oct 1, 2022

rmartin16 commented Oct 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freakboy3742 commented Oct 2, 2022

mhsmith commented Oct 2, 2022

freakboy3742 commented Oct 2, 2022

Choose a reason for hiding this comment

freakboy3742 commented Oct 3, 2022

rmartin16 commented Oct 3, 2022

codecov bot commented Oct 3, 2022

Codecov Report

freakboy3742 commented Oct 3, 2022

saroad2 commented Oct 3, 2022

freakboy3742 commented Sep 29, 2022 •

edited

rmartin16 commented Sep 30, 2022 •

edited