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

Actually use Impeller in scenario_app tests #50977

Merged
merged 8 commits into from
Feb 27, 2024

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented Feb 26, 2024

Right now, the scenario_app tests that claim to use Impeller are not actually using Impeller. This is for a few reasons:

  • The arguments passed via instrumentation do not end up by default on the Intent for the Activity that is under test.
  • The arguments passed via instrumentation were in the wrong order and not getting sent to instrumentation at all.

This patch updates existing tests to use a new @Rule that reads the arguments from the instrumentation's argument Bundle and injects them into the Intent that we actually pass to the Activity under test. This patch updates the test runner to force initialization of the shell arguments for all tests once and early. It also updates the argument order in the script and adds a verification that Impeller prints that it is being used at least once.

@dnfield dnfield requested a review from matanlurey February 26, 2024 20:22
Copy link
Contributor

@matanlurey matanlurey left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for doing this!

@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 26, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Feb 26, 2024
Copy link
Contributor

auto-submit bot commented Feb 26, 2024

auto label is removed for flutter/engine/50977, due to - The status or check suite Linux linux_android_aot_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

@matanlurey
Copy link
Contributor

@dnfield I am going to hit "Update branch" so we can get my logging improvements and Jonah's crash improvements.

@dnfield
Copy link
Contributor Author

dnfield commented Feb 26, 2024

Something is going wrong in the general case - Impeller is getting used when I run a single test, but not when I run them all.

@dnfield
Copy link
Contributor Author

dnfield commented Feb 27, 2024

I've modified this patch to be a bit simpler and remove a test that does not work with Impeller currently - the test tries to construct a Flutter Engine in an unusual way and ends up hanging when we try to initialize Vulkan for reasons that are not entirely clear to me. I don't think we need to do that though, we have coverage already elsewhere around initializing FlutterEngine objects.

@dnfield dnfield requested a review from matanlurey February 27, 2024 01:57
@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 #50977 at sha 0b852a2

@matanlurey
Copy link
Contributor

I took a quick look at the gold digests and they look WAI, the texture tests that are rendering blank are waiting for a combination of #50730 (h/t @chinmaygarde) and flutter/flutter#144199 (h/t @johnmccutchan).

@dnfield
Copy link
Contributor Author

dnfield commented Feb 27, 2024

I'm going to approve the goldens. Chinmay can merge this into his PR and that should give him the test coverage he needs.

@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 27, 2024
@auto-submit auto-submit bot merged commit 4addb6a into flutter:main Feb 27, 2024
@dnfield dnfield deleted the scenario_app_args branch February 27, 2024 04:39
auto-submit bot pushed a commit that referenced this pull request Feb 27, 2024
Follow up on #50977 - enables OpenGLES backend for that test harness.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App will affect goldens
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants