Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore enhancements #454

Merged
merged 9 commits into from
Jul 6, 2023
Merged

Conversation

kturney
Copy link
Contributor

@kturney kturney commented Jun 19, 2023

Adds some enhancements to ignore comment handling:

  1. allow omitting space between // and coverage in coverage ignore comments
  2. allow text after coverage ignore comments
  3. throw FormatException when encountering unbalanced ignore comments instead of silently erroring

Hopefully 1 and 2 are uncontroversial.
3 did change an existing test.
We found it quite confusing in our projects when seemingly ignored lines were still appearing as uncovered.
Only after much investigation would various devs discover that it was due to accidental nesting of ignore ranges.
To my mind, the coverage tool should lean towards being permissive (or maybe hard fail?).
Potentially an analyzer rule could be added to detect unbalanced coverage ignore comments.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

CHANGELOG.md Outdated Show resolved Hide resolved
@liamappelbe
Copy link
Contributor

We found it quite confusing in our projects when seemingly ignored lines were still appearing as uncovered.
Only after much investigation would various devs discover that it was due to accidental nesting of ignore ranges.
To my mind, the coverage tool should lean towards being permissive (or maybe hard fail?).

Sounds reasonable overall, but I actually lean towards a hard fail. You could just throw a FormatException with the file name and line number.

At the very least we should report a warning. This package doesn't really have any logging infrastructure yet though, so maybe just print a warning message to stderr. The cleaner solution would be to use package:logging, if you don't mind the extra effort.

@kturney
Copy link
Contributor Author

kturney commented Jun 22, 2023

@liamappelbe hard fail sounds good to me. Seems like it would be more obvious what the issue is in CI environments, especially with noisy logs.

I'll move forward with adding the FormatException unless I hear back that you'd prefer logging.

@kturney
Copy link
Contributor Author

kturney commented Jul 4, 2023

Changed to throwing FormatException with the file name and line number for unmatched coverage ignore comments

@coveralls
Copy link

coveralls commented Jul 4, 2023

Coverage Status

coverage: 93.434% (+0.06%) from 93.379% when pulling 7c15965 on kturney:ignore-enhancements into 523cf4d on dart-lang:master.

Copy link
Contributor

@liamappelbe liamappelbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Almost ready.

for (final content in invalidSources) {
expect(getIgnoredLines(content.split('\n')), isEmpty);
test('throws FormatException when the annotations are not balanced', () {
for (var i = 0; i < invalidSources.length; ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The structure of this test is a bit odd. Can you refactor it like this:

test('throws FormatException when the annotations are not balanced', () {
  runTest(String source, String errMsg) {
    final content = invalidSources[i].split('\n');
    expect(...);
  }

  runTest(invalidSources[0], 'coverage:ignore-start found at content-0.dart:3'
                ' before previous coverage:ignore-start ended');
  runTest(invalidSources[1], 'coverage:ignore-start found at content-1.dart:3'
                ' before previous coverage:ignore-start ended');
  ...
});

@liamappelbe liamappelbe merged commit 19f7189 into dart-lang:master Jul 6, 2023
9 checks passed
@kturney kturney deleted the ignore-enhancements branch July 8, 2023 04:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants