-
Notifications
You must be signed in to change notification settings - Fork 54
Add --ignore-files option to exclude files from code coverage using path patterns #496
Add --ignore-files option to exclude files from code coverage using path patterns #496
Conversation
@@ -124,50 +127,54 @@ Future<void> main(List<String> arguments) async { | |||
Environment parseArgs(List<String> arguments) { | |||
final parser = ArgParser(); | |||
|
|||
parser.addOption('sdk-root', abbr: 's', help: 'path to the SDK root'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
careful making unrelated changes. Makes reviews harder.
bin/format_coverage.dart
Outdated
@@ -66,6 +68,7 @@ Future<void> main(List<String> arguments) async { | |||
final hitmap = await HitMap.parseFiles( | |||
files, | |||
checkIgnoredLines: env.checkIgnore, | |||
ignoreGlobs: env.ignoreFiles, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add this logic in the same place as reportOn
? You could just add it to the _getPathFilter
function in formatter.dart. Having the logic in two totally different places makes it a bit hard to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first I wanted to put it in the same place where // coverage:ignore-file
is handled, I forgot about reportsOn
. I moved the logic to _getPathFilter
, it's much better now. Additionally the ignore-files
option can now be used without check-ignore
flag, which I think makes more sense than the previous approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Just need to tighten up the tests a bit.
ignoreGlobs: {Glob('**/*.g.dart'), Glob('**/util.dart')}, | ||
); | ||
|
||
expect(res, isNot(contains(p.absolute(_sampleGeneratedPath)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should be able to add an extra check for this file to all the other tests in this file: expect(res, contains(p.absolute(_sampleGeneratedPath)));
. That way you're checking both the contains and doesn't contain cases.
At the moment I think these tests could pass accidentally by, for example, mistyping the _sampleGeneratedPath
, since every test that checks it is checking that it's not in the report.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this check as well as some additional checks in globs related tests to make the expected behaviour more explicit
…ath patterns (dart-archive/coverage#496) * Add --ignore-files option, update tests * Simplify checking if any glob matches the source * Move logic to _getPathFilter * Remove unused variable, avoid excessive calls to canonicalize * Flatten ifs * Avoid exceeding line length limit * Add missing expects * Remove junk
This makes it possible to exclude files which match glob patterns from the report, which was discussed in dart-lang/tools#510 and dart-lang/tools#463. The
.yaml
configuration part which was requested in dart-lang/tools#510 has not been implemented, but it can be done separately after this is merged.