Skip to content

Parameterize CoverageCollector with a library name predicate#36774

Merged
jiahaog merged 1 commit intoflutter:masterfrom
jiahaog:parameterize-coverage-predicate
Jul 24, 2019
Merged

Parameterize CoverageCollector with a library name predicate#36774
jiahaog merged 1 commit intoflutter:masterfrom
jiahaog:parameterize-coverage-predicate

Conversation

@jiahaog
Copy link
Member

@jiahaog jiahaog commented Jul 23, 2019

An optimization to the coverage collection speed was added in #30811. This commit further expands on it to parameterize the CoverageCollector with a custom predicate, allowing internal use cases to filter the RPC calls to the Dart VM based on scripts of interest to coverage collection.

@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Jul 23, 2019

It looks like this pull request may not have tests. Please make sure to add tests before merging. While there are exceptions to this rule, if this patch modifies code it is probably not an exception.

Reviewers: Read the Tree Hygine page and make sure this patch meets those guidelines before LGTMing.

/cc @dnfield

@fluttergithubbot fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jul 23, 2019
@jiahaog jiahaog marked this pull request as ready for review July 23, 2019 18:01
@jiahaog jiahaog requested a review from jonahwilliams July 23, 2019 18:01
@jonahwilliams
Copy link
Contributor

jonahwilliams commented Jul 23, 2019

@jiahaog can you take a look at the analyzer failures?

@jiahaog
Parameterize CoverageCollector with a library name predicate
4d4c7a5 
An optimization to the coverage collection speed was added in #30811. This commit further expands on it to parameterize the CoverageCollector with a custom predicate, allowing internal use cases to filter the RPC calls to the Dart VM based on scripts of interest to coverage collection.
@jiahaog
Copy link
Member Author

jiahaog commented Jul 23, 2019

My bad, I wasn't familiar with how the checks work.

@codecov
Copy link

codecov bot commented Jul 24, 2019 •
edited
Loading

Codecov Report

Merging #36774 into master will decrease coverage by 0.15%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #36774      +/-   ##
==========================================
- Coverage   55.88%   55.73%   -0.16%     
==========================================
  Files         190      190              
  Lines       17590    17580      -10     
==========================================
- Hits         9831     9798      -33     
- Misses       7759     7782      +23
Flag Coverage Δ
#flutter_tool 55.73% <40%> (-0.16%) ⬇️
Impacted Files Coverage Δ
packages/flutter_tools/lib/src/commands/test.dart 19.8% <0%> (-0.2%) ⬇️
...flutter_tools/lib/src/test/coverage_collector.dart 61.42% <66.66%> (+0.46%) ⬆️
...ackages/flutter_tools/lib/src/resident_runner.dart 45.18% <0%> (-8.04%) ⬇️
packages/flutter_tools/lib/src/base/io.dart 70.58% <0%> (-2.95%) ⬇️
packages/flutter_tools/lib/src/version.dart 93.59% <0%> (-1.98%) ⬇️
...ckages/flutter_tools/lib/src/ios/code_signing.dart 96.96% <0%> (-1.52%) ⬇️
packages/flutter_tools/lib/src/vmservice.dart 38.81% <0%> (-0.96%) ⬇️
...kages/flutter_tools/lib/src/commands/assemble.dart 64.86% <0%> (-0.93%) ⬇️
packages/flutter_tools/lib/src/emulator.dart 69.49% <0%> (-0.85%) ⬇️
packages/flutter_tools/lib/src/cache.dart 47.05% <0%> (-0.77%) ⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b4d9f6...4d4c7a5. Read the comment docs.

jonahwilliams
jonahwilliams approved these changes Jul 24, 2019
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

To ensure this doesn't regress coverage time can you verify that the flutter_test performance doesn't regress with this change? https://flutter-dashboard.appspot.com/benchmarks.html.

This can be tested with:

cd dev/devicelab
dart bin/run.dart -t flutter_test_performance.dart

@jiahaog
Copy link
Member Author

jiahaog commented Jul 24, 2019 •
edited
Loading

Do those benchmarks depend on the platform I'm running them on (I'm running them on MacOS)? I ran them and they are all slightly faster compared to those on https://flutter-dashboard.appspot.com/benchmarks.html.

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Jul 24, 2019

The benchmarks are run on an overheated Linux box. You'd need to compare before/after on the same machine

@jiahaog
Copy link
Member Author

jiahaog commented Jul 24, 2019

The benchmarks are relatively similar, I think this change is safe to merge:

$ dart bin/run.dart -t bin/tasks/flutter_test_performance.dart

{
  "success": true,
  "data": {
    "without_change_elapsed_time_ms": 2993,
    "implementation_change_elapsed_time_ms": 6436,
    "interface_change_elapsed_time_ms": 6131,
    "with_coverage_time_ms": 3161
  },
  "benchmarkScoreKeys": [
    "without_change_elapsed_time_ms",
    "implementation_change_elapsed_time_ms",
    "interface_change_elapsed_time_ms",
    "with_coverage_time_ms"
  ]
}

$ git checkout master

$ dart bin/run.dart -t bin/tasks/flutter_test_performance.dart

{
  "success": true,
  "data": {
    "without_change_elapsed_time_ms": 3227,
    "implementation_change_elapsed_time_ms": 6467,
    "interface_change_elapsed_time_ms": 6442,
    "with_coverage_time_ms": 3263
  },
  "benchmarkScoreKeys": [
    "without_change_elapsed_time_ms",
    "implementation_change_elapsed_time_ms",
    "interface_change_elapsed_time_ms",
    "with_coverage_time_ms"
  ]
}

@jiahaog jiahaog merged commit 4de9b44 into flutter:master Jul 24, 2019
@jiahaog jiahaog deleted the parameterize-coverage-predicate branch July 24, 2019 17:58
johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019
@jiahaog @johnsonmh
Parameterize CoverageCollector with a library name predicate (flutter…
f4138f1 
…#36774)

An optimization to the coverage collection speed was added in flutter#30811. This commit further expands on it to parameterize the CoverageCollector with a custom predicate, allowing internal use cases to filter the RPC calls to the Dart VM based on scripts of interest to coverage collection.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jiahaog @fluttergithubbot @jonahwilliams @googlebot