Use Flutter tool to build and test the scenario app

[blasten]

This simplifies the workflow. It allows to test JIT & AOT builds using the Flutter Gradle plugin.
It would also be great if assemble_apk.sh is just flutter build apk, but that would require setting ENGINE_PATH and ANDROID_SDK_ROOT in the recipe.

Also cc @nturgut since I'm using tools/clone_flutter.sh.

[dnfield]

This change should not land.

These tests were created explicitly to not depend on the Flutter framework or tool. There are two goals for that:

1. Validate that the engine/dart:ui can be used without the Flutter framework/tool.
2. Avoid creating a cyclic dependency on build/test for the engine and framework repos.

[blasten]

@dnfield

What is the use case for dart:ui without the framework? Can that be resolved if the framework isn’t imported in lib/main.dart?

I’m not sure if it’s worth the complexity. I’m trying to debug the app, and it’s difficult in the current state. I need a JIT build because I only have the emulator available at the moment.

Also, the scenario app on Android seems to be failing already. I’m not sure why, but I was going to investigate.

About the cyclic dependency, what is the situation that we are trying to avoid?

[dnfield]

We should not have any dependencies in the engine repository on anything from the flutter repository. That is cyclic and will break for reasons that become hard to trace for contributors.

We should just write the right scripts to run jit mode on Android. Using Gradle is fine too.

[blasten]

That is cyclic and will break for reasons that become hard to trace for contributors.

What is an example? tools/clone_flutter.sh fetches a version of the framework that was committed before the latest engine commit.

We should just write the right scripts to run jit mode on Android. Using Gradle is fine too.

This change is really just using Gradle, but it includes the Flutter Gradle plugin (which calls flutter assemble). I worked on the plugin, and it broke many times because there wasn't this level of testing with a local engine.

[xster]

I think it's helpful to break this discussion into 2 parts.

I agree with not using the Flutter framework in the engine tests since they're not the things actively under test. (Though we'll want full e2e tests at some point like web as well).

This is for the tools. Since the engine repo doesn't have any JIT/AOT building tooling under test that we're trying to isolate, I think that's ok.

Whether we use the framework or not, breakages are likely real bugs. Not using the framework reduces the debugging surface area when there are real breaking bugs. Not using the tools doesn't necessarily reduce the debugging surface area when there are real bugs (since bugs that happen in our parallel building scripts aren't "real" production bugs).

I think the main intent here is to make breakages more easily actionable rather than avoiding breakages. i.e. the pain of having to indirectly fix an engine breakage is conceptually favorable to not breaking. Though in practice, I'm pretty worried that tools/clone_flutter.sh just gets the last version rather than trying to get the latest green version. We'd want to avoid spurious blockages on engine work for bugs that are already detected.

[blasten]

@dnfield We have a similar situation between the Flutter and the Plugins repos. I remember landing changes to Gradle that momentarily broke the Plugins repo. The process wasn't ideal, but useful for detecting real breakages. I reverted my changes and re-landed with the appropriate fix. Could we have a similar policy here?

[dnfield]

We don't run ci here on commits to flutter. It will get missed until e.g. an autoroller fails and then skia is backed up for hours and we miss regressions there.

We do test the framework elsewhere, and that does cause problems from time to time. We should work to reduce that surface rather than expand it.

And if we make it easier for future contributors to start importing flutter into this app, more context is lost about why that isn't helpful here.

/Cc @chinmaygarde @cbracken. If they're ok with taking this on I'll withdraw my concern.

[blasten]

Also /cc @amirh who mentioned about moving the android_views test to the engine. https://github.com/flutter/flutter/tree/master/dev/integration_tests/android_views

[xster]

@dnfield that's kinda what I was saying above. If it's going to fail here first, it's likely going to be a case where there's a real bug for a scenario that isn't tested in the framework's own tests (such as building local engines) that we happen to catch here rather than being reported by our customers (who do use local engines).

If the framework breaks customer: engine TODAY, we should treat it in that same generic way. Add more tests in the framework to catch things that were somehow caught in an engine integration test with framework tools. If we're just making everything fail slightly earlier and not substituting some non-bugs or some earlier bugs with later bugs (which is what you want to avoid), I think that's a positive thing.

That said, tools/clone_flutter.sh just getting the last (potentially red) framework revision will produce non-bug breakages.

[chinmaygarde]

I concur with Dan on all points.

On the cyclic dependencies specifically: The Flutter Engine is at neither the floor or the roof of the Flutter tech stack. This makes affordances for testing the entire stack and not just the engine in isolation very hard to reason about (more on this below). The focus on treating the entire stack as a single unit used to work well when Flutter was a simpler project. However, this also led to insufficient attention being paid to devising stable interfaces between the various layers of the stack, and more importantly, testing those interfaces. In turn, because the interfaces were neither well defined nor tested, resorting to end-to-end tests became more of a necessity. Now, end-to-end tests are necessary but not sufficient to determine engine viability. Since the sum total of all the variations in which the Flutter stack can be configured includes the undefined ways in which the layers interact with one another, an end-to-end test can only ever make sure to test a single overall configuration, but, cannot tell if that configuration was constructed using undefined/untested/shifting interactions between one of more layers. The philosophical argument to not having cyclic dependencies was strengthening the focus on the devising and testing interfaces between the various layers of the stack. Philosophical tedium aside, the ability to configure the stack using undefined interactions between layers makes it so that a change in the framework that depends on these undefined interactions will now go uncaught till an end-to-end test in the engine is run, and, not at the point at which the offending interaction was introduced. If this test were added to an engine presubmit, folks working on the engine are now somewhat helpless on what they did to cause the breakage, when in fact, the breakage happened long ago in a dependent. This was the case with the recent framework presubmit check that led to many engineers losing valuable productivity and led to Chris retiring that presubmit entirely.

Xiao is right that having these breakages being caught at all is valuable. So, I don’t want to get in the way of folks writing tests that depend on the framework. Feel free to add a scenarios app substitute in the framework repo that depends on the framework itself. Then you can add a test to the engine scenarios app that tests engine interfaces and another test in the framework scenarios app that tests how the frameworks uses those interfaces. That should just lead to us having to write more tests which works fine by me.

Hope this colors the thought process on the policy against cyclic dependencies.

Specific answers to other questions:

What is an example? tools/clone_flutter.sh fetches a version of the framework that was committed before the latest engine commit.

@blasten: See the discussion on breakages not being caught when they were introduced in the framework and the loss of productivity caused by the framework presubmits.

Whether we use the framework or not, breakages are likely real bugs. Not using the framework reduces the debugging surface area when there are real breaking bugs.

@xster: Agreed. Does the alternative where there is a separate scenarios app substitute in the framework satisfy your concerns. Using a single test harness for everything is in no way a requirement. And a policy that forces more tests just seems like a win-win.

Also /cc @amirh who mentioned about moving the android_views test to the engine.

If that test depends on the framework, the same argument against introducing a cyclic dependency will hold unless it is simplified to not depend on the framework.

[cbracken]

@dnfield: This change should not land.

Agreed. As @dnfield says, we want to minimise circular dependencies between the engine and framework.

@blasten: What is the use case for dart:ui without the framework? Can that be resolved if the framework isn’t imported in lib/main.dart?

The goal of the engine is to expose a fast, portable, lightweight runtime on which the framework is built. We tell users that they should use as much or as little of the framework as fits their needs. One extreme of that is no framework. Both inside and outside Google, we have users who don't depend on the tool. (e.g. customer: dream).

More importantly than the philosophical arguments here are the pragmatic ones. For better or for worse, the engine exposes two pieces of public API: dart:ui and the embedding API. We need to validate that the engine makes no assumptions about those APIs that hide bugs. I can think of a crasher that lay hidden in flow for years due to all use going through the framework.

@xster: Whether we use the framework or not, breakages are likely real bugs. Not using the framework reduces the debugging surface area when there are real breaking bugs. Not using the tools doesn't necessarily reduce the debugging surface area when there are real bugs (since bugs that happen in our parallel building scripts aren't "real" production bugs).

I hate to suggest more repos/infra, but one answer here would be to break the tool into its own repo. Long term there are potential benefits to that, like making platform-specific support something that happens in a pluggable way. Users implement an embedder and a tool plugin for their platform, but that specific discussion is out of scope here. I also think that's unlikely to be viable in the short term, and contribute to more infra woes. In the short term, I think @chinmaygarde's suggestion of having a separate case for this is reasonable.

[cbracken]

Notes above -- replying to trigger the right github APIs :)

[blasten]

If this test were added to an engine presubmit, folks working on the engine are now somewhat helpless on what they did to cause the breakage, when in fact, the breakage happened long ago in a dependent.

Could that be solved if we treat the Flutter repo like any other gn dependency? It probably wouldn't need to be updated very frequently. @chinmaygarde

I hate to suggest more repos/infra, but one answer here would be to break the tool into its own repo. Long term there are potential benefits to that, like making platform-specific support something that happens in a pluggable way. @cbracken

That would be ideal. However, given the current situation, if we ensure that each invocation of the flutter tool has the --offline flag set (it ignores engine.version) and it's required to be used with --local-engine. Could that solved the problem of cyclic dependencies?

What we would have is just the flutter tool which is upgraded via DEPS and it always uses the local engine artifacts. This could be the closest approximation to having a separate repo for the tool.

framework tests in the engine

I agree. The scenario app doesn't depend on the framework. If we are concerned about someone accidentally doing that, we could have a linter check.

I heard everyone concern. I'm currently blocked without a physical device. I also found that this test isn't running in either CIs, and it broke in #17473:

418  1447 I flutter : Observatory listening on http://127.0.0.1:36156/V5o0OS0ftOM=/
04-27 18:39:44.714  1418  1437 E flutter : [ERROR:flutter/lib/ui/ui_dart_state.cc(157)] Unhandled Exception: 'package:scenario_app/main.dart': Failed assertion: line 48 pos 10: 'window.locale != null': is not true.
04-27 18:39:44.714  1418  1437 E flutter : #0      _AssertionError._doThrowNew (dart:core-patch/errors_patch.dart:42:39)
04-27 18:39:44.714  1418  1437 E flutter : #1      _AssertionError._throwNew (dart:core-patch/errors_patch.dart:38:5)
04-27 18:39:44.714  1418  1437 E flutter : #2      main (package:scenario_app/main.dart:48:10)
04-27 18:39:44.714  1418  1437 E flutter : #3      _runMainZoned.<anonymous closure>.<anonymous closure> (dart:ui/hooks.dart:257:25)
04-27 18:39:44.714  1418  1437 E flutter : #4      _rootRun (dart:async/zone.dart:1184:13)
04-27 18:39:44.714  1418  1437 E flutter : #5      _CustomZone.run (dart:async/zone.dart:1077:19)
04-27 18:39:44.714  1418  1437 E flutter : #6      _runZoned (dart:async/zone.dart:1619:10)
04-27 18:39:44.714  1418  1437 E flutter : #7      runZonedGuarded (dart:async/zone.dart:1608:12)
04-27 18:39:44.714  1418  1437 E flutter : #8      _runMainZoned.<anonymous closure> (dart:ui/hooks.dart:249:5)
04-27 18:39:44.714  1418  1437 E flutter : #9      _startIsolate.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:301:19)
04-27 18:39:44.714  1418  1437 E flutter : #10     _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:168:12)
04-27 18:39:44.714  1418  1437 E flutter :

I need to switch between different configurations to test different behavior. I find myself looking at the tool code to try to simulate a similar environment in the engine. This was my motivation behind reusing the Flutter tool.

[xster]

I generally agree with @cbracken and @chinmaygarde's point. Though some are discussing things that are not directly related to this PR. Specifically about not wanting the exercise the engine as an "under test" target with the framework.

1. I agree
2. That's not what this PR is for
3. It's a good discussion to have but it should be had in Run integration tests on ci #17598 (it already landed)

About having a mock framework to test the engine's stable APIs, we're all in agreement and that's what @dnfield created with the scenarios test.

This PR is just about deduplicating our ad-hoc scripts that packages the mock framework with the engine in the same way as how we package our real framework with the engine. i.e. the engine is not "under test". If a PR that rm -rf packages/flutter in the flutter/flutter repo got merged, it shouldn't break this PR.

On points that relate to this PR:

• The idea of adding lints or checks to the whole tools/clone_flutter.sh mechanism to make sure we only depend on things in the flutter_tools is a good idea.
• Splitting flutter_tools into its own repo is good too but that's a long term project.
• Not blocking google rolls due to a flutter_tools breakage is a good point since google doesn't use flutter_tools. That's a good decoupling argument, though I think the advantage of decoupling is already not present. i.e. a broken flutter_tools already blocks a roll.

[chinmaygarde]

having a mock framework to test the engine's stable APIs, we're all in agreement and that's what @dnfield created with the scenarios test.

Correction (@xster): There is no mock framework being used in the Scenarios app created by Dan. All tests use dart:ui directly. So I don't believe we are all in agreement about what the scenarios test is. Maybe that is the cause of the confusion? The scenario's app only tests the dart:ui interfaces.

Could that be solved if we treat the Flutter repo like any other gn dependency?

@blasten: An improvement, but one that causes its own issues. How is that dependency going to rolled for example?

This PR is just about deduplicating our ad-hoc scripts that packages the mock framework with the engine in the same way as how we package our real framework with the engine.

@xster: I am not sure I see it that way. Consider that this may be viewed as an elaboration of an already ad-hoc script (//testing/scenario_app/run_android_tests.sh) but one that now depends on the framework as well. It doesn't really get more ad-hoc than starting local maven repos., etc.. Your point about the framework stuff not being used to test the engine is well received though.

I think our discussion has graduated from analyzing whether the framework should be used to write e2e tests in the engine (it shouldn't) and towards if this patch is an improvement to the ad-hoc script used to launch the scenarios app. I contend that even the current scripts are brittle and an extension of the same unwise. @dnfield might have thoughts on how to make these better.

It's a good discussion to have but it should be had in #17598 (it already landed)

I wasn't on that review but such techniques have proven to be incredibly flaky and brittle. Especially when dealing with branches, tags, and hot-fixes. I'd wait and see if that attempt leads to any unnecessary tree closures or roadblocks in rolls. It's definitely not a strategy we want to commit to just because its already commited for a portion of the targets.

[xster]

Correction (@xster): There is no mock framework being used in the Scenarios app created by Dan. All tests use dart:ui directly. So I don't believe we are all in agreement about what the scenarios test is. Maybe that is the cause of the confusion? The scenario's app only tests the dart:ui interfaces.

We're saying the same thing. The Dart code in the scenario cases talking to dart:ui directly are simulating a fake framework, i.e. mocks.

I think our discussion has graduated from analyzing whether the framework should be used to write e2e tests in the engine (it shouldn't) and towards if this patch is an improvement to the ad-hoc script used to launch the scenarios app. I contend that even the current scripts are brittle and an extension of the same unwise. @dnfield might have thoughts on how to make these better.

Yes! This is what we should be discussing. This new one's better IMO. The existing scripts are incomplete (as @blasten is pointing out) and if those scripts specifically fail, it doesn't signal anything. If the new scripts fail, it's a meaningful signal for us and our customers (besides google).

I wasn't on that review but such techniques have proven to be incredibly flaky and brittle. Especially when dealing with branches, tags, and hot-fixes. I'd wait and see if that attempt leads to any unnecessary tree closures or roadblocks in rolls. It's definitely not a strategy we want to commit to just because its already commited for a portion of the targets.

I already agreed with you 🤣

[blasten]

How is that dependency going to rolled for example?

As needed. If someone needs a newer version, they can bump the version. Just like buildroot: #17978. - The nice thing about that is that the cost is paid by the person interested in a newer version, which seems like a fair tradeoff.

There have been cases in the past where local-engine is broken in the Flutter tool. I filed flutter/flutter#39762 a while ago to remind myself about the pain that it caused.

It would also help fix flutter/flutter#29840, and flutter/flutter#38933 (you filed that one :-) ), and potentially flutter/flutter#51989.

Since most folks use flutter --local-engine to test changes to the engine, why not use that machinery to test the scenario app?

By exercising these paths, we can ensure that the tool (with --local-engine) works as expected. At the same time, simplifying the tooling and reducing the cognitive load for newer contributors.

The scenario app has complex logic at this point. The platform view scenarios are complex, and likely to get more complex with Hybrid composition coming to Android. It would be a time saver if the tool could be used to dump the layer tree for example, or connect to the debugger.

Consider that this may be viewed as an elaboration of an already ad-hoc script (//testing/scenario_app/run_android_tests.sh)

I agree. In fact, this is a great exercise about what should be changed in the Flutter tool to make the functionality more reusable and testable.

I hope this gives a bit more context about the rationale.

wdyt @chinmaygarde, @cbracken ?

Could we please come up with a conclusion? What would I need to change about my current proposal?

So far, I got:

1. The flutter repo is added to DEPS and updated manually similar to buildroot.
2. In the CI, the flutter tool cannot download artifacts, so it's determinatic. It operates with --offline. Therefore, it can only be used with --local-engine.

[xster]

I like the idea of adding more lints to prevent accidental scope creep from this dependency (if we can somehow disable the tool in non --offline mode). Even better if we can use sparse checkout to only check out the Flutter tool without the Flutter framework.

[cbracken]

Since this PR is pretty stale at this point and has merge conflicts, I'm going to close for now. @blasten if you'd like to revive this work later, I'm happy to review!