Add a java injector for testing #20789
Conversation
xster
requested review from
mehmetf,
jason-simmons and
GaryQian
and removed request for
liyuqian
|
Oh that's a good point. Thanks. I don't see any getInstance uses in google3 but just to be safe, deprecated for now. Looking at getInstanceForTest, it seems like it's not really what it says it does. I don't think we should have added it in the first place either. Refactored the test that uses it to just use mockito spy which is a lot simpler anyway. |
xster
force-pushed
the
java-test-injector
branch
from
5986342
to
4d40e3f
Compare
xster
added
the
waiting for tree to go green
|
This pull request is not suitable for automatic merging in its current state.
|
fluttergithubbot
removed
the
waiting for tree to go green
| @@ -252,7 +252,8 @@ public void notifyLowMemoryWarning() { | |||
| public static class DartEntrypoint { | |||
| @NonNull | |||
| public static DartEntrypoint createDefault() { | |||
| return new DartEntrypoint(FlutterMain.findAppBundlePath(), "main"); | |||
| return new DartEntrypoint( | |||
| FlutterInjector.instance().flutterLoader().findAppBundlePath(), "main"); | |||
There was a problem hiding this comment.
This still implies findAppBundlePath needs to return a default if FlutterLoader is not inited.
There was a problem hiding this comment.
This is somewhat implicit but the DartExecutor is created by the engine and only creates it after having loaded the native library. If I understood your question correctly. We can add asserts and stuff too to make the dependency explicit.
There was a problem hiding this comment.
Yes, we should add asserts since this is public API IIRC.
There was a problem hiding this comment.
Turns out this needs a sizable refactor 🤣 3ce3419
PTAL
| FlutterLoader.Settings newSettings = new FlutterLoader.Settings(); | ||
| newSettings.setLogTag(settings.getLogTag()); | ||
| FlutterLoader.getInstance().startInitialization(applicationContext, newSettings); | ||
| FlutterInjector.instance().flutterLoader().startInitialization(applicationContext, newSettings); |
There was a problem hiding this comment.
Might be less verbose to expose helper functions to fetch loader from the injector. Sort of like how modules work in DI frameworks.
There was a problem hiding this comment.
sure, it's a good idea. I'm currently leaning towards leaving things as simple as possible rather than having a production DI system since it's mostly for us and I want to keep it with as few indirections as possible for now.
| @@ -30,43 +30,17 @@ public void pluginsCanAccessFlutterAssetPaths() { | |||
| FlutterApplicationInfo emptyInfo = | |||
| new FlutterApplicationInfo(null, null, null, null, null, null, false, false); | |||
| return; | ||
| } | ||
| FlutterLoader.getInstance().ensureInitializationComplete(applicationContext, args); | ||
| FlutterInjector.instance() |
There was a problem hiding this comment.
This is a breaking change... Apps testing via robolectric will need to do something else to avoid the native library load such as using a mockito spy and overriding loadNativeLibrary.
There was a problem hiding this comment.
Ya, I thought it might be as well, though I'm not seeing anyone using it cs/isrunninginrobolectrictest. Am I searching it wrong? I'm happy to let https://github.com/flutter/engine/pull/20789/files#diff-8e0c9afcddc7151854497b73e0484fccR92 read out of something in the flutterinjector as well.
There was a problem hiding this comment.
I am not against the change :) We just need to document it. You are right, I don't think g3 is using this but someone else might.
There was a problem hiding this comment.
Our new thing is that breaking changes are things that break tests 😐 https://flutter.dev/docs/resources/compatibility
xster
force-pushed
the
java-test-injector
branch
from
a7b5d07
to
efcc33e
Compare
|
I'm gonna submit this so I can chain my next PR. Windows passed on luci, not sure why it's not reflecting on github. |
|
I did end up breaking a test in the plugins repo T.T Breaking change doc flutter/website#4561 |
I was working on https://github.com/flutter/engine/pull/20769/files#diff-1e3150b8858cc1cf6e4f83a2c5172e1aR372 and couldn't test it. We're reaching the limit of scalability for ad-hoc constructor dependency injection especially if it needs to leak the dependencies through the whole plumbing.
Make an injector to make testing easier. Start with the FlutterLoader and swap its singleton getter to use the testable injector.