Skip to content

Add --cache-sksl flag to drive and run #42353

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.

Sign up for GitHub

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

Jump to bottom
Merged
liyuqian merged 2 commits into from
Oct 15, 2019
Merged

Add --cache-sksl flag to drive and run #42353

liyuqian merged 2 commits into from
Oct 15, 2019

Conversation

@liyuqian
Copy link
Contributor

@liyuqian liyuqian commented Oct 9, 2019

So we can test SkSL precompile using the command line tools.
See flutter/engine#12412.

@liyuqian liyuqian requested a review from jonahwilliams October 9, 2019 19:53
@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Oct 9, 2019

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

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

@fluttergithubbot fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Oct 9, 2019
jonahwilliams
jonahwilliams reviewed Oct 9, 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.

This needs tests:

There should be some existing test coverage that asserts that command line arguments are turned into the correct adb/process command. I think that would be all that is needed in this case

@liyuqian
Add --cache-sksl flag to drive and run
137e06c 
So we can test SkSL precompile using the command line tools.
See flutter/engine#12412.
@liyuqian
Copy link
Contributor Author

liyuqian commented Oct 9, 2019

Thank you @jonahwilliams for the quick comment! Can you please give me a link to the existing test that I can reference?

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Oct 9, 2019

Hmm, I was sure what existing test cases that cover these. At any rate, what you'll likely want are two tests.

One for iOS in https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/general.shard/ios/devices_test.dart which calls startApp and ensures that the command line args are correctly set. Specifically, something like https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/general.shard/ios/devices_test.dart#L172 but with assertions on the command line arguments.

Another one for Android in https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/general.shard/android/android_device_test.dart . Unfortunately we don't have an existing one, but it would look much like the iOS tests.

To run a single test in the tools repo, you can run bin/cache/dart-sdk/bin/pub run test path/to/test

@codecov
Copy link

codecov bot commented Oct 9, 2019 •
edited
Loading

Codecov Report

Merging #42353 into master will decrease coverage by 0.12%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #42353      +/-   ##
==========================================
- Coverage   59.94%   59.81%   -0.13%     
==========================================
  Files         194      194              
  Lines       18871    18997     +126     
==========================================
+ Hits        11313    11364      +51     
- Misses       7558     7633      +75
Flag Coverage Δ
#flutter_tool 59.81% <88.88%> (-0.13%) ⬇️
Impacted Files Coverage Δ
packages/flutter_tools/lib/src/commands/drive.dart 75.2% <100%> (+0.2%) ⬆️
packages/flutter_tools/lib/src/commands/run.dart 65.38% <100%> (+0.5%) ⬆️
packages/flutter_tools/lib/src/ios/devices.dart 66.52% <100%> (-8.77%) ⬇️
packages/flutter_tools/lib/src/device.dart 60% <100%> (-5.67%) ⬇️
.../flutter_tools/lib/src/android/android_device.dart 50.8% <66.66%> (+17%) ⬆️
...lutter_tools/lib/src/build_system/targets/ios.dart 4.25% <0%> (-74.47%) ⬇️
...kages/flutter_tools/lib/src/commands/generate.dart 20% <0%> (-65.19%) ⬇️
packages/flutter_tools/lib/src/web/workflow.dart 33.33% <0%> (-55.56%) ⬇️
packages/flutter_tools/lib/src/web/web_device.dart 19.27% <0%> (-28.92%) ⬇️
...ages/flutter_tools/lib/src/linux/linux_device.dart 44.44% <0%> (-26.99%) ⬇️
... and 54 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 3fea283...73817e0. Read the comment docs.

@liyuqian
Copy link
Contributor Author

liyuqian commented Oct 11, 2019

@jonahwilliams : tests added.

@liyuqian liyuqian force-pushed the sksl_warmup branch 2 times, most recently from 8d4f837 to a4fc249 Compare October 12, 2019 00:42
@liyuqian
Copy link
Contributor Author

liyuqian commented Oct 15, 2019

@jonahwilliams : friendly ping...

jonahwilliams
jonahwilliams approved these changes Oct 15, 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

@liyuqian liyuqian merged commit 31cb448 into flutter:master Oct 15, 2019
@liyuqian liyuqian deleted the sksl_warmup branch October 18, 2019 17:47
@liyuqian liyuqian mentioned this pull request Oct 22, 2019
Closed
3 tasks
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
@liyuqian @Inconnu08
Add --cache-sksl flag to drive and run (flutter#42353)
62ee0de 
So we can test SkSL precompile using the command line tools.
See flutter/engine#12412.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 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

@liyuqian @fluttergithubbot @jonahwilliams @googlebot