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

Improve, test, and fix a bug related to adb logcat filtering. #51012

Merged

Conversation

matanlurey
Copy link
Contributor

  1. Write tests for the AdbLogLine extension/parsing, and run it on CI.
    Fixes Add a test of the ADB log parsing flutter#144164.

  2. Improve the logging to include androidemu-related errors logs
    Work towards Add a test of the ADB log parsing flutter#144164.

  3. Clarified logic/fixed a bug related to handling filterProcessId: ...

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.

LGTM with some nits

static const Set<String> kKnownNoiseTags = <String>{
'MonitoringInstr',
'ResourceExtractor',
'THREAD_STATE',
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Is this really a noise tag? This gets dumped if a test hangs and tells you what the threads are doing right? Is it used otherwise.

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 moved this from kKnownNoise to kKnownUsefulError.


void main() {
/// Simulates the filtering of logcat output [lines].
Iterable<String> filter(Iterable<String> lines, {int? filterProcessId}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: does this really belong in its own testable class? Right now this looks like it's roughly copied from the run_android_test.dart

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'll file a follow-up issue, as it really depends on how much more work I'm going to do on the CLI:
flutter/flutter#144252

@flutter-dashboard
Copy link

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #51012 at sha 20bb6e1

@matanlurey matanlurey merged commit 0f727a5 into flutter:main Feb 27, 2024
23 checks passed
@matanlurey matanlurey deleted the improve-and-test-adb-log-filtering branch February 27, 2024 22:31
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 28, 2024
64a375de9c [Windows] Make the engine own a map of views (flutter/engine#51017)
73480bf11d Rename some classes in the engine_build_configs package (flutter/engine#51016)
2756f14a46 Remove rewrapper prefix from compiler commands for clang-tidy (flutter/engine#51001)
0f727a5b31 Improve, test, and fix a bug related to `adb logcat` filtering. (flutter/engine#51012)
ab4d6db3f1 Move vulkan-deps to //flutter/third_party/vulkan-deps (flutter/engine#51013)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants