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

Better filtering for Android scenario_app runner. #50937

Merged
merged 8 commits into from
Feb 26, 2024

Conversation

matanlurey
Copy link
Contributor

@matanlurey matanlurey commented Feb 24, 2024

🍴 'd from #50933, will rebase when merged.

Closes flutter/flutter#143458.

A picture is a 1000 words:

Screenshot 2024-02-23 at 7 01 29 PM

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.

@matanlurey matanlurey marked this pull request as ready for review February 26, 2024 17:32
Copy link
Member

@gaaclarke gaaclarke left a 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*(.*)');
Copy link
Member

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.

Copy link
Contributor Author

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;
Copy link
Member

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.

Copy link
Contributor Author

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();
Copy link
Member

@gaaclarke gaaclarke Feb 26, 2024

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.

Copy link
Contributor Author

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');
Copy link
Member

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").

Copy link
Contributor Author

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)
Copy link
Member

@gaaclarke gaaclarke Feb 26, 2024

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.

Copy link
Contributor Author

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!

Copy link
Contributor

@dnfield dnfield left a 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.

@dnfield
Copy link
Contributor

dnfield commented Feb 26, 2024

(but LGTM)

@matanlurey matanlurey merged commit b28cfbb into flutter:main Feb 26, 2024
23 checks passed
@matanlurey matanlurey deleted the scenario_app-better-filtering branch February 26, 2024 19:36
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 27, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 27, 2024
…144208)

flutter/engine@04ff286...0bc21ea

2024-02-27 matanlurey@users.noreply.github.com Respect SIGINT (Ctrl-C) for Android scenario_app. (flutter/engine#50989)
2024-02-27 skia-flutter-autoroll@skia.org Roll Skia from aa28c3a30a98 to 2f2a718b27f7 (1 revision) (flutter/engine#50998)
2024-02-27 skia-flutter-autoroll@skia.org Roll Dart SDK from 2876f5684ced to 67b2a250747b (1 revision) (flutter/engine#50996)
2024-02-27 matanlurey@users.noreply.github.com Fix usage of `--out-dir` with a relative path. (flutter/engine#50992)
2024-02-27 jason-simmons@users.noreply.github.com Roll buildroot to 21b1b9f2645fada701885108e86aefbcb3b1cca0 (flutter/engine#50991)
2024-02-27 skia-flutter-autoroll@skia.org 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 matanlurey@users.noreply.github.com Migrate Android `scenario_app` to the `SurfaceProducer` API (flutter/engine#50993)
2024-02-27 dkwingsmt@users.noreply.github.com Revert "Reland 4: Multiview pipeline (#50931)" (flutter/engine#50985)
2024-02-26 matanlurey@users.noreply.github.com Refactor args parsing/environment constructor for `scenario_app` (flutter/engine#50980)
2024-02-26 jonahwilliams@google.com [scenario] trigger firstFrameLatch on exception. (flutter/engine#50981)
2024-02-26 jason-simmons@users.noreply.github.com [Impeller] Fix a misspelling and name mismatch in a shader test fixture (flutter/engine#50983)
2024-02-26 skia-flutter-autoroll@skia.org Roll Dart SDK from c479735adcf9 to 2876f5684ced (2 revisions) (flutter/engine#50979)
2024-02-26 zanderso@users.noreply.github.com Run engine unit tests on mac host_debug_unopt_arm64 (flutter/engine#50327)
2024-02-26 jonahwilliams@google.com [Impeller] disble render pass caches. (flutter/engine#50976)
2024-02-26 john@johnmccutchan.com Update Surface reference after resizing render target in VirtualDisplay based platform views (flutter/engine#50971)
2024-02-26 jason-simmons@users.noreply.github.com [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 matanlurey@users.noreply.github.com Better filtering for Android `scenario_app` runner. (flutter/engine#50937)
2024-02-26 jacksongardner@google.com Make sure to call `setHeightOverride` as well on TextStyle and StrutStyle (flutter/engine#50920)
2024-02-26 jacksongardner@google.com Correctly offset the cull rect of the opacity layer. (flutter/engine#50928)
2024-02-26 skia-flutter-autoroll@skia.org 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 jimgraham@google.com,rmistry@google.com,zra@google.com 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants