Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Reland Android systrace#29080

Merged
dnfield merged 8 commits into
flutter-team-archive:masterfrom
dnfield:android_systrace
Oct 8, 2021
Merged

Reland Android systrace#29080
dnfield merged 8 commits into
flutter-team-archive:masterfrom
dnfield:android_systrace

Conversation

@dnfield

@dnfield dnfield commented Oct 8, 2021

Copy link
Copy Markdown
Contributor

Adding more logging to see if I can figure out what's going on on the bot.

@flutter-dashboard

flutter-dashboard Bot commented Oct 8, 2021

Copy link
Copy Markdown

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 on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

@google-cla google-cla Bot added the cla: yes label Oct 8, 2021
@dnfield dnfield requested a review from zanderso October 8, 2021 16:43
@dnfield

dnfield commented Oct 8, 2021

Copy link
Copy Markdown
Contributor Author

@zanderso - I had a successful run here: https://luci-milo.appspot.com/raw/build/logs.chromium.org/flutter/led/dnfield_google.com/a5e0ef9eeb08d87f7a542b482f782c281ce20eada7392a93c5a05697870b3c12/+/build.proto

I've added some more logging so that if this fails again we'll know why. I also added a 10 minute timeout on the part where we wait for the observatory to be available to avoid tying up the bot for longer than that.

zanderso
zanderso reviewed Oct 8, 2021
View reviewed changes
Comment thread testing/android_systrace_test.py Outdated
break
# If it takes longer than 10 minutes to find the observatory, avoid tying up
# the bot.
logcat.wait(timeout=(10 * 60))

@zanderso zanderso Oct 8, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't follow how this works. Isn't this line only reached if the logcat process dies or the loop on 62 finds the Observatory line?

@zanderso zanderso Oct 8, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Either way, if this code doesn't work, we'll have to fix it as a P0 whether the bot is taking 60 minutes or 10 minutes to fail, so I'd recommend skipping the timeout here.

@dnfield dnfield Oct 8, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hm. I think yo'ure right. I had this in here at one point when I wasn't setting up the popen correctly and it seemed to help, but now I think I was wrong.

I'll just remove the line entirely.

@dnfield dnfield Oct 8, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Rerunning led to be sure: https://ci.chromium.org/swarming/task/5679c2230fabac10?server=chromium-swarm.appspot.com

@zanderso

zanderso commented Oct 8, 2021

Copy link
Copy Markdown
Contributor

The logs for the test run show a Java exception. Is that expected:

>>>>>>>> 10-08 16:36:34.678  7011  7011 E GeneratedPluginsRegister: Tried to automatically register plugins with FlutterEngine (io.flutter.embedding.engine.FlutterEngine@1306a8a) but could not find or invoke the GeneratedPluginRegistrant.

>>>>>>>> 10-08 16:36:34.683  7011  7011 E GeneratedPluginsRegister: Received exception while registering

>>>>>>>> 10-08 16:36:34.683  7011  7011 E GeneratedPluginsRegister: java.lang.ClassNotFoundException: io.flutter.plugins.GeneratedPluginRegistrant

@dnfield

dnfield commented Oct 8, 2021

Copy link
Copy Markdown
Contributor Author

Yes - scenario app does not have plugins. We should probably fix that.

@dnfield

dnfield commented Oct 8, 2021

Copy link
Copy Markdown
Contributor Author

This one looks like it's hanging. Will try to figure out what's going on here.

@zanderso

zanderso commented Oct 8, 2021

Copy link
Copy Markdown
Contributor

Yes - scenario app does not have plugins. We should probably fix that.

I'm surprised that a scary looking exception in the logs is part of normal operation here. It makes it seem like the app might be broken or misconfigured. @blasten @stuartmorgan

@dnfield

dnfield commented Oct 8, 2021

Copy link
Copy Markdown
Contributor Author

Normally the Flutter tool would generate an empty GeneratedPluginRegistrant and the exception wouldn't happen. The Scenario app doesn't have it because it's not created by the tool.

@dnfield

dnfield commented Oct 8, 2021

Copy link
Copy Markdown
Contributor Author

I think I've figured out the problem - sometimes LeakCanary's starting an activity instead of the one we actually want. I'm trying to use a different method that will not allow that to happen. Latest run is at

https://ci.chromium.org/swarming/task/567a8666de29d410?server=chromium-swarm.appspot.com

zanderso
zanderso approved these changes Oct 8, 2021
View reviewed changes

@zanderso zanderso left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

still lgtm

Comment thread testing/android_systrace_test.py

print('Trace did not contain ShellSetupUISubsystem, failing.')
return 1

@zanderso zanderso Oct 8, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: missing newline

@dnfield dnfield Oct 8, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@dnfield dnfield added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Oct 8, 2021
@dnfield dnfield merged commit 541779d into flutter-team-archive:master Oct 8, 2021
@dnfield dnfield deleted the android_systrace branch October 8, 2021 22:55
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 9, 2021
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 9, 2021
@engine-flutter-autoroll
541779d Reland Android systrace (flutter-team-archive/engine#29080)
32aeea4
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 9, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 9, 2021
@engine-flutter-autoroll
541779d Reland Android systrace (flutter-team-archive/engine#29080)
45637a4
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@zanderso zanderso zanderso approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes needs tests platform-android waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dnfield @zanderso