-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Better filtering for Android scenario_app
runner.
#50937
Better filtering for Android scenario_app
runner.
#50937
Conversation
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.
LGTM. Just a few suggestions for clarity and versatility.
// | ||
// This regex is simple versus being more precise. Feel free to improve it. | ||
static final RegExp _pattern = RegExp(r'([^A-Z]*)([A-Z])\s([^:]*)\:\s(.*)'); | ||
static final RegExp _pattern = RegExp(r'(\d+-\d+\s[\d|:]+\.\d+)\s+(\d+)\s+(\d+)\s(\w)\s(\S+)\s*:\s*(.*)'); |
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.
Can you put those comments inline in the regex? I'm not sure if the dart regex parser supports them.
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 cannot unfortunately (Dart regex support lacking).
@@ -311,27 +327,26 @@ Future<void> _run({ | |||
final (Future<int> logcatExitCode, Stream<String> logcatOutput) = getProcessStreams(logcatProcess); | |||
|
|||
logcatProcessExitCode = logcatExitCode; | |||
String? filterToProcessId; |
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 found this name really confusing. The gist is that it is the process id we are filtering for right filterProcessId
seems more clear to me.
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.
Yeah I agree, cleaned it up a bit and added comments.
return; | ||
} | ||
|
||
filterToProcessId ??= adbLogLine.tryParseProcess(); |
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.
So the intention is no printing until we've parsed a process? It might be worth adding a comment here for that.
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.
Agreed, done.
} | ||
|
||
void logWarning(String msg) { | ||
stderr.writeln('$_yellow$msg$_reset'); |
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.
nit: a function that takes in a color would be nice so not everyone has to remember to reset the color. colorLog(stderr, _yellow, "hello")
.
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.
Groovy! Done.
/// `adb logcat` and explain what log tag names are most common. | ||
void main(List<String> args) { | ||
if (args case [final String path]) { | ||
final List<AdbLogLine> parsed = io.File(path) |
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.
nit: Ideally this tool would work by streaming from stdout. That way it wouldn't be limited by memory size and you could say stuff like tail foo.log | logcat_reader
or adb logcat | logcat_reader
.
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.
Yeah this is intended to read from our stored logcat.txt, not necessarily the live process. I edited the tool to make that more clear!
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.
Consider adding some test logs that parse the way you want them to so that we know in the future what the logcat you were basing this on looked like.
(but LGTM) |
…144208) flutter/engine@04ff286...0bc21ea 2024-02-27 [email protected] Respect SIGINT (Ctrl-C) for Android scenario_app. (flutter/engine#50989) 2024-02-27 [email protected] Roll Skia from aa28c3a30a98 to 2f2a718b27f7 (1 revision) (flutter/engine#50998) 2024-02-27 [email protected] Roll Dart SDK from 2876f5684ced to 67b2a250747b (1 revision) (flutter/engine#50996) 2024-02-27 [email protected] Fix usage of `--out-dir` with a relative path. (flutter/engine#50992) 2024-02-27 [email protected] Roll buildroot to 21b1b9f2645fada701885108e86aefbcb3b1cca0 (flutter/engine#50991) 2024-02-27 [email protected] Roll Skia from ba3ed5998af3 to aa28c3a30a98 (12 revisions) (flutter/engine#50994) 2024-02-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Migrate Android `scenario_app` to the `SurfaceProducer` API (#50993)" (flutter/engine#50995) 2024-02-27 [email protected] Migrate Android `scenario_app` to the `SurfaceProducer` API (flutter/engine#50993) 2024-02-27 [email protected] Revert "Reland 4: Multiview pipeline (#50931)" (flutter/engine#50985) 2024-02-26 [email protected] Refactor args parsing/environment constructor for `scenario_app` (flutter/engine#50980) 2024-02-26 [email protected] [scenario] trigger firstFrameLatch on exception. (flutter/engine#50981) 2024-02-26 [email protected] [Impeller] Fix a misspelling and name mismatch in a shader test fixture (flutter/engine#50983) 2024-02-26 [email protected] Roll Dart SDK from c479735adcf9 to 2876f5684ced (2 revisions) (flutter/engine#50979) 2024-02-26 [email protected] Run engine unit tests on mac host_debug_unopt_arm64 (flutter/engine#50327) 2024-02-26 [email protected] [Impeller] disble render pass caches. (flutter/engine#50976) 2024-02-26 [email protected] Update Surface reference after resizing render target in VirtualDisplay based platform views (flutter/engine#50971) 2024-02-26 [email protected] [Impeller] Fix a race that can abort the process if the Vulkan context is destroyed while pipeline creation tasks are pending (flutter/engine#50883) 2024-02-26 [email protected] Better filtering for Android `scenario_app` runner. (flutter/engine#50937) 2024-02-26 [email protected] Make sure to call `setHeightOverride` as well on TextStyle and StrutStyle (flutter/engine#50920) 2024-02-26 [email protected] Correctly offset the cull rect of the opacity layer. (flutter/engine#50928) 2024-02-26 [email protected] Roll Fuchsia Linux SDK from kLvCWEgbL1VTRW69e... to JCdhkDSFXzHyPuP4I... (flutter/engine#50970) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from kLvCWEgbL1VT to JCdhkDSFXzHy If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
🍴 'd from #50933, will rebase when merged.
Closes flutter/flutter#143458.
A picture is a 1000 words:
This is still noisy, but at least all the output appears to be part of the execution.
As you recall, the full logs are always available in the FLUTTER_LOGS_DIR output.