Let first frame listeners self remove #11014

xster · 2019-08-15T00:14:59Z

See test:

Let the listener callbacks remove themselves for a one-time callback without java.util.ConcurrentModificationException.

matthew-carroll · 2019-08-15T01:02:27Z

shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java

+  private static class FlutterJNITest extends FlutterJNI {
+    protected void onFirstFrame() {
+      // Exposed here for tests.
+      super.onFirstFrame();


Why is this here?

Discussed offline. Probably won't need this class once I move the package up one level at which point it'll be in the same package as the FlutterJNI.

matthew-carroll · 2019-08-15T01:03:22Z

shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java

+  @Before
+  public void setUp() {
+    jniUnderTest = new FlutterJNITest();
+    rendererUnderTest = new FlutterRenderer(jniUnderTest);


I recommend instantiating the object under test within each test. You never know when a test is going to be introduced that requires some kind of unusual instantiation, and when it is, that developer will be forced to change every other test to make that happen. Better to just let each test configure the object under test as it desired.

Also, I don't think we need to suffix things with "test"...everything in here is related to a test.

I think the first point was to not use test class setup/teardowns? It's probably a bit philosophical but I think it'll be efficient to just use industry standard practices (such as cs/@before$ file:test.java)

I think this case is a bit exceptional but most of the time, there'd be things that are under test and a bunch more things that are mocks supporting those real classes under test. This makes it clear which are actors in the test and which aren't (e.g. cs/"undertest =" file:test.java) but I don't feel strongly about it.

matthew-carroll · 2019-08-15T01:04:23Z

shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java

+  @Test
+  public void firstFrameListenerCanRemoveThemselves() {
+    AtomicInteger callbackCalled = new AtomicInteger(0);
+    OnFirstFrameRenderedListener callback = new OnFirstFrameRenderedListener() {


I think you can probably accomplish this with a mock with less code and complexity than what you have here...

mockito is unfortunately a bit less capable for these level triggered stuff (mostly due to java)

An equivalent here will turn into

@Mock OnFirstFrameRenderedListener callback; when(callback. onFirstFrameRendered()).thenAnswer( new Answer() { public Object answer(InvocationOnMock invocation) { rendererUnderTest.removeOnFirstFrameRenderedListener(callback); } });

which is even less directly readable.

matthew-carroll · 2019-08-15T01:05:25Z

shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java

+  }
+
+  @Test
+  public void firstFrameListenerCanRemoveThemselves() {


I'd like to follow a policy whereby any true unit test prefixes the test with "it", e.g., "itAllowsFirstFrameListenersToRemoveThemselves". This pattern will help ensure that unit tests really are unit tests by grammatically framing in terms of the object under test.

Also, this isn't really testing the broader capability of removing oneself as a listener. This is specifically testing the capabilityof removing oneself during a first frame callback. You may want to make that explicit in the test name.

ok. Renamed.

I think the only way to not remove themselves during the first frame would be something like itAllowsFirstFrameListenersToRemoveThemselvesWithoutUsingAHandlerToPostBackADeferredRemoval :D

How's itAllowsFirstFrameListenersToRemoveThemselvesInline?

matthew-carroll · 2019-08-15T01:08:38Z

shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java

+    };
+
+    rendererUnderTest.addOnFirstFrameRenderedListener(callback);
+    jniUnderTest.onFirstFrame();


I would recommend that in all tests we clearly demarcate between setup/execution/verification. In this case everything up to and including the addOnFirstFrameRenderedListener is part of the setup, the onFirstFrame() invocation is the execution, and everything that follows is verification.

In addition to placing a blank line and maybe a comment between these 2 lines above, you could also slightly refactor what comes below. I dont think you actually need to interleave 2 onFirstFrame()s and 2 asserts. You can simply invoke onFirstFrame() twice in a row, and then verify that the callback was invoked exactly 1 time.

I think it's still worth checking for the actual timing (e.g. that we don't have false positives on onFirstFrame only calling listeners on second invocation or something)

matthew-carroll · 2019-08-15T01:10:18Z

shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java

    ensureRunningOnMainThread();
    if (renderSurface != null) {
      renderSurface.onFirstFrameRendered();
    }
    // TODO(mattcarroll): log dropped messages when in debug mode (https://github.com/flutter/flutter/issues/25391)
-
-    for (OnFirstFrameRenderedListener listener : firstFrameListeners) {
+    CopyOnWriteArrayList<OnFirstFrameRenderedListener> mutableListeners =


This leads me to believe that this is not a test of FlutterRenderer but rather a test of FlutterJNI. I think something needs to change in this PR to achieve correctness in that regard. If you're truly unit testing FlutterRenderer then you shouldn't have to make this protected. If you're testing FlutterJNI then the test suite should be switched accordingly.

I think there's a couple of layers of testings we'll need either way.

I think we'll need strict unit tests. Where we're testing classes. I added a FlutterJNITest for this one.

Some of the classes are just not part of the public API are not going to be used. We should have medium tests that tests things between an API surface and some sort of abstraction layer. In this case, I think making things that platform_view_android_jni.cc calls also test callable probably makes sense.

We need integration tests that also includes C++ stuff but since those are heavier, they won't be per feature.

matthew-carroll · 2019-08-15T01:10:54Z

shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java

+import java.util.concurrent.atomic.AtomicInteger;
+
+@Config(manifest=Config.NONE)
+@RunWith(RobolectricTestRunner.class)


Why does this need to be robolectric?

FlutterRenderer imports android classes

xster · 2019-08-22T20:11:42Z

cc @RedBrogdon since you wanted to follow along

matthew-carroll · 2019-09-03T22:50:25Z

I think we can close this PR due to related changes made here:
#11357

xster · 2019-09-03T22:57:34Z

yup, thanks for re-doing all the tests

googlebot added the cla: yes label Aug 15, 2019

xster requested a review from matthew-carroll August 15, 2019 00:41

matthew-carroll reviewed Aug 15, 2019

View reviewed changes

xster added 3 commits August 20, 2019 10:58

let listener9s self remove

bb2fe0c

forgot to name it right

3b87bd8

add another test for flutter jni

a9a9247

xster force-pushed the listener-self-remove branch from 037b7e3 to a9a9247 Compare August 20, 2019 21:53

xster added 2 commits August 20, 2019 15:19

rename tests

52b30c9

name tweak

738d555

xster closed this Sep 3, 2019

xster deleted the listener-self-remove branch September 3, 2019 22:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let first frame listeners self remove #11014

Let first frame listeners self remove #11014

xster commented Aug 15, 2019 •

edited

Loading

matthew-carroll Aug 15, 2019

xster Aug 20, 2019

matthew-carroll Aug 15, 2019

matthew-carroll Aug 15, 2019

xster Aug 20, 2019

matthew-carroll Aug 15, 2019

xster Aug 20, 2019

matthew-carroll Aug 15, 2019

matthew-carroll Aug 15, 2019

xster Aug 20, 2019

matthew-carroll Aug 15, 2019

xster Aug 20, 2019

matthew-carroll Aug 15, 2019

xster Aug 20, 2019

matthew-carroll Aug 15, 2019

xster Aug 20, 2019

xster commented Aug 22, 2019

matthew-carroll commented Sep 3, 2019

xster commented Sep 3, 2019

Let first frame listeners self remove #11014

Let first frame listeners self remove #11014

Conversation

xster commented Aug 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xster commented Aug 22, 2019

matthew-carroll commented Sep 3, 2019

xster commented Sep 3, 2019

xster commented Aug 15, 2019 •

edited

Loading