Add --fail-under flag for minimum coverage threshold #2075
Conversation
Signed-off-by: vvs-personalstash <[email protected]>
Signed-off-by: vvs-personalstash <[email protected]>
| } | ||
| } | ||
|
|
||
| int precision; |
There was a problem hiding this comment.
What's this option for? Was there a reason you decided to add it?
There was a problem hiding this comment.
It was present in the coverage.py implementation of fail-under to accurately match the coverage percentage in decimals while printing and therefore i added it as it seemed like a good feature for better cosmetics.If its not required i can remove it. An example is like if we have a 93.267% of actual coverage and we do --fail-under=93.3 it will print Error: Coverage 93.0% is less than required 93.3% but if we do --precision=1 we will have an output of Error: Coverage 93.2% is less than required 93.3%
There was a problem hiding this comment.
Seems unnecessary to me. I think just printing 2 decimal places should be fine.
There was a problem hiding this comment.
Hmm it seems you are right I will just round it up to 2 places then, I shall begin working on your proposed changes.Thank You.
There was a problem hiding this comment.
No, I mean I wouldn't really bother rounding at all. Just print 2 decimal places when you're printing the unrounded number
There was a problem hiding this comment.
Oh yes, that makes more sense — I will do that. Thank you.
There was a problem hiding this comment.
Hi Liam, Sorry for asking you about this again but I have thought about it more and think that isn't it more convenient to use the --precision as well as rounding off upto our precision i.e. round up to the precision value for the users of the package because:-
- If we compare hard values while enforcing threshold then in cases such as lets say e.g 94.999 and 95 are almost same and we wont need our CI to fail because of this.
- In medium and small projects 1 extra line covered leads to drastic change in percentages hence we can keep precision on a low or =0 value as even a difference of 0.1 maybe acceptable to users in CI i.e they would want a 95.4 to pass if they keep the threshold 95.5
- In large projects we can keep a more precise threshold but still we wont want our CI to fail at 94.4999 if our threshold is 95.5.
Therefore I still propose to keep the --precision and rounding off to precision in the implementation as thresholding will provide users more explicit control on how strongly they want to enforce their threshold for nearby values rather than them adding tests to cover 1 more line or constantly changing the -fail-under value.
[I just saw I didn't mention about the rounding off upto precision in the first comment sorry about that]
I maybe making some wrong assumptions or inferences in this and would really like your opinion on this matter again.Thank you.
There was a problem hiding this comment.
Isn't choosing a threshold of X with a precision of Y is the same as choosing a threshold of X-Y?
- If we compare hard values while enforcing threshold then in cases such as lets say e.g 94.999 and 95 are almost same and we wont need our CI to fail because of this.
You'd still have a hard threshold. If precision=0.1, you're just shifting the hard threshold from 95 to 94.9
If the project is healthy, then the coverage should be significantly above whatever threshold they've set, so a change of a few lines won't matter. I think of this flag as a safety net to warn them when they've been lax in their testing for a while, and need a concerted effort of test writing to bring their coverage ratio back up. If users want more fine grained monitoring they should use Coveralls.
There was a problem hiding this comment.
Thanks For Your explanation. I have implemented the requested changes and I am working on the integration tests
| @@ -160,6 +173,29 @@ ${parser.usage} | |||
| ); | |||
| } | |||
|
|
|||
| double? failUnder; | |||
There was a problem hiding this comment.
This is duplicated logic with the formatter. Maybe remove it and pass through the flag string unparsed? The formatter step will take care of parsing and validation.
| var coveredLines = 0; | ||
|
|
||
| for (final entry in hitmap.entries) { | ||
| final lineHits = entry.value.lineHits; |
There was a problem hiding this comment.
Can you also include the branchHits, if any? It's possible to have 100% line coverage but miss some branches. No need to include the funcHits as they are a subset of the lineHits.
There was a problem hiding this comment.
The unit test that checks the calculateCoveragePercentage function is a good start, but we'll also need an integration test to verify that the check's logic works: one test that verifies that the command succeeds when the coverage percentage is above the threshold, and test that verifies it fails when the coverage is below the threshold. There's a few existing integration tests that you can piggy back on. Maybe test_with_coverage_test.dart, but there's others too.
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
PR HealthBreaking changes ✔️
Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs.
Coverage
|
| File | Coverage |
|---|---|
| pkgs/coverage/bin/format_coverage.dart | 💔 45 % ⬇️ 8 % |
| pkgs/coverage/bin/test_with_coverage.dart | 💚 36 % ⬆️ 1 % |
| pkgs/coverage/lib/src/coverage_percentage.dart | 💚 100 % |
This check for test coverage is informational (issues shown here will not fail the PR).
This check can be disabled by tagging the PR with skip-coverage-check.
API leaks ✔️
The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
| Package | Leaked API symbols |
|---|
License Headers ✔️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
| Files |
|---|
| no missing headers |
All source files should start with a license header.
Unrelated files missing license headers
| Files |
|---|
| pkgs/bazel_worker/benchmark/benchmark.dart |
| pkgs/bazel_worker/example/client.dart |
| pkgs/bazel_worker/example/worker.dart |
| pkgs/benchmark_harness/integration_test/perf_benchmark_test.dart |
| pkgs/boolean_selector/example/example.dart |
| pkgs/clock/lib/clock.dart |
| pkgs/clock/lib/src/clock.dart |
| pkgs/clock/lib/src/default.dart |
| pkgs/clock/lib/src/stopwatch.dart |
| pkgs/clock/lib/src/utils.dart |
| pkgs/clock/test/clock_test.dart |
| pkgs/clock/test/default_test.dart |
| pkgs/clock/test/stopwatch_test.dart |
| pkgs/clock/test/utils.dart |
| pkgs/coverage/lib/src/coverage_options.dart |
| pkgs/coverage/test/collect_coverage_config_test.dart |
| pkgs/coverage/test/config_file_locator_test.dart |
| pkgs/html/example/main.dart |
| pkgs/html/lib/dom.dart |
| pkgs/html/lib/dom_parsing.dart |
| pkgs/html/lib/html_escape.dart |
| pkgs/html/lib/parser.dart |
| pkgs/html/lib/src/constants.dart |
| pkgs/html/lib/src/encoding_parser.dart |
| pkgs/html/lib/src/html_input_stream.dart |
| pkgs/html/lib/src/list_proxy.dart |
| pkgs/html/lib/src/query_selector.dart |
| pkgs/html/lib/src/token.dart |
| pkgs/html/lib/src/tokenizer.dart |
| pkgs/html/lib/src/treebuilder.dart |
| pkgs/html/lib/src/utils.dart |
| pkgs/html/test/dom_test.dart |
| pkgs/html/test/parser_feature_test.dart |
| pkgs/html/test/parser_test.dart |
| pkgs/html/test/query_selector_test.dart |
| pkgs/html/test/selectors/level1_baseline_test.dart |
| pkgs/html/test/selectors/level1_lib.dart |
| pkgs/html/test/selectors/selectors.dart |
| pkgs/html/test/support.dart |
| pkgs/html/test/tokenizer_test.dart |
| pkgs/pubspec_parse/test/git_uri_test.dart |
| pkgs/stack_trace/example/example.dart |
| pkgs/watcher/test/custom_watcher_factory_test.dart |
| pkgs/yaml_edit/example/example.dart |
Pull Request Test Coverage Report for Build 14562796465Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Signed-off-by: vvs-personalstash <[email protected]>
Implement
--fail-underFlag for Enforcing Minimum Code Coverage ThresholdsThis pull request introduces a new command-line flag,
--fail-under, to thepackage:coveragetool. The feature is modeled after the behavior of the--fail-underflag in Python’scoverage.pylibrary.Overview of Changes
New Feature:
--fail-underexit(1)), signaling failure.Optional Precision Control:
--precision--precisionflag has been introduced to control the number of decimal places shown in the computed coverage percentage.Need Clarity
-Currently i have only added tests for directly testing the coverage percentage calculator,Is it sufficient or should I modify the test_with_coverage_test to test the flag also
Fixes #514