-
Notifications
You must be signed in to change notification settings - Fork 27.4k
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
Android log reader reads any recent logs #45743
Conversation
ProcessManager: () => mockProcessManager, | ||
}); | ||
}); | ||
|
||
test('Can parse adb shell dumpsys info', () { |
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.
we should probably also have a real devicelab test that verifies that we're not getting old logs when you run flutter run
or flutter logs
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.
definitely
final List<String> args = <String>['shell', '-x', 'logcat', '-v', 'time', '-s', 'flutter']; | ||
final String lastTimestamp = device.lastLogcatTimestamp; | ||
// Start the adb logcat process and filter the most recent logs since `lastTimestamp`. | ||
final List<String> args = <String>['logcat', '-v', 'time', '-T', lastTimestamp]; | ||
processUtils.start(device.adbCommandForDevice(args)).then<void>((Process process) { |
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.
this looks a lot simpler than the code used to look; why was the old code more complicated?
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'm not sure. I was trying to see in which version the -T
flag was introduced.
Approach sounds reasonable to me. |
Description
After #45307, the Android log reader doesn't filter older logs. Instead, it filters by tag and the protocol discovery class throttles the logs to discover the most recent observatory URI. The intention with this change was to allow
flutter attach
to attach to an app already running on the device. However, this implementation detail is used by the microbenchmarks test to extract the benchmark data.This PR reverts to the old behavior. Going forward, we can have two factories for the Android log reader such the one that filters by tags is explicitly injected in the
flutter attach
flow and the other one is injected influtter run/logs
flows. cc @xsterRelated Issues
Fixes #45732
Tests
I added the following tests: Unit test
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze --flutter-repo
) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?