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

Let first frame listeners self remove #11014

Closed
xster wants to merge 5 commits into from
Closed

Let first frame listeners self remove #11014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bb2fe0c
let listener9s self remove
xster Aug 15, 2019
3b87bd8
forgot to name it right
xster Aug 15, 2019
a9a9247
add another test for flutter jni
xster Aug 20, 2019
52b30c9
rename tests
xster Aug 20, 2019
738d555
name tweak
xster Aug 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions shell/platform/android/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,8 @@ action("robolectric_tests") {
"test/io/flutter/embedding/android/FlutterActivityTest.java",
"test/io/flutter/embedding/android/FlutterFragmentTest.java",
"test/io/flutter/embedding/engine/FlutterEngineCacheTest.java",
"test/io/flutter/embedding/engine/FlutterJNITest.java",
"test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java",
"test/io/flutter/embedding/engine/systemchannels/TextInputChannelTest.java",
"test/io/flutter/util/PreconditionsTest.java",
]
Expand Down
10 changes: 7 additions & 3 deletions shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,14 @@
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
import android.support.annotation.UiThread;
import android.support.annotation.VisibleForTesting;
import android.view.Surface;
import android.view.SurfaceHolder;

import java.nio.ByteBuffer;
import java.util.HashSet;
import java.util.Set;
import java.util.concurrent.CopyOnWriteArrayList;

import io.flutter.Log;
import io.flutter.embedding.engine.FlutterEngine.EngineLifecycleListener;
Expand Down Expand Up @@ -275,15 +277,17 @@ public void removeOnFirstFrameRenderedListener(@NonNull OnFirstFrameRenderedList

// Called by native to notify first Flutter frame rendered.
@SuppressWarnings("unused")
@VisibleForTesting
@UiThread
private void onFirstFrame() {
protected void onFirstFrame() {
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 =
Copy link
Contributor

@matthew-carroll matthew-carroll Aug 15, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@xster xster Aug 20, 2019

Choose a reason for hiding this comment

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

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.

new CopyOnWriteArrayList<>(firstFrameListeners);
for (OnFirstFrameRenderedListener listener : mutableListeners) {
listener.onFirstFrameRendered();
}
}
Expand Down
4 changes: 4 additions & 0 deletions shell/platform/android/test/io/flutter/FlutterTestSuite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
import io.flutter.embedding.android.FlutterActivityTest;
import io.flutter.embedding.android.FlutterFragmentTest;
import io.flutter.embedding.engine.FlutterEngineCacheTest;
import io.flutter.embedding.engine.FlutterJNITest;
import io.flutter.embedding.engine.renderer.FlutterRendererTest;
import io.flutter.embedding.engine.systemchannels.TextInputChannelTest;
import io.flutter.util.PreconditionsTest;

Expand All @@ -23,6 +25,8 @@
FlutterFragmentTest.class,
// FlutterActivityAndFragmentDelegateTest.class, TODO(mklim): Fix and re-enable this
FlutterEngineCacheTest.class,
FlutterJNITest.class,
FlutterRendererTest.class,
TextInputChannelTest.class
})
/** Runs all of the unit tests listed in the {@code @SuiteClasses} annotation. */
Expand Down
57 changes: 57 additions & 0 deletions shell/platform/android/test/io/flutter/embedding/engine/FlutterJNITest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
package io.flutter.embedding.engine;

import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.robolectric.RobolectricTestRunner;
import org.robolectric.annotation.Config;

import io.flutter.embedding.engine.FlutterJNI;
import io.flutter.embedding.engine.renderer.FlutterRenderer;
import io.flutter.embedding.engine.renderer.OnFirstFrameRenderedListener;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.mock;

import java.util.concurrent.atomic.AtomicInteger;

@Config(manifest=Config.NONE)
@RunWith(RobolectricTestRunner.class)
public class FlutterJNITest {
FlutterJNI jniUnderTest;

@Before
public void setUp() {
jniUnderTest = new FlutterJNI();
}

@Test
public void itAllowsFirstFrameListenersToRemoveThemselvesInline() {
// --- Test Setup ---
AtomicInteger callbackCalled = new AtomicInteger(0);
OnFirstFrameRenderedListener callback = new OnFirstFrameRenderedListener() {
@Override
public void onFirstFrameRendered() {
callbackCalled.incrementAndGet();
jniUnderTest.removeOnFirstFrameRenderedListener(this);
};
};
jniUnderTest.addOnFirstFrameRenderedListener(callback);

// --- Execute Test ---
jniUnderTest.onFirstFrame();

// --- Verify Results ---
assertEquals(1, callbackCalled.get());

// --- Execute Test ---
// The callback removed itself from the listener list. A second call doesn't call the callback.
jniUnderTest.onFirstFrame();

// --- Verify Results ---
assertEquals(1, callbackCalled.get());
}
}
66 changes: 66 additions & 0 deletions shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
package io.flutter.embedding.engine.renderer;

import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.robolectric.RobolectricTestRunner;
import org.robolectric.annotation.Config;

import io.flutter.embedding.engine.FlutterJNI;
import io.flutter.embedding.engine.renderer.FlutterRenderer;
import io.flutter.embedding.engine.renderer.OnFirstFrameRenderedListener;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.mock;

import java.util.concurrent.atomic.AtomicInteger;

@Config(manifest=Config.NONE)
@RunWith(RobolectricTestRunner.class)
Copy link
Contributor

@matthew-carroll matthew-carroll Aug 15, 2019

Choose a reason for hiding this comment

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

Why does this need to be robolectric?

Copy link
Member Author

@xster xster Aug 20, 2019

Choose a reason for hiding this comment

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

FlutterRenderer imports android classes

public class FlutterRendererTest {
FlutterJNITest jniUnderTest;
FlutterRenderer rendererUnderTest;

@Before
public void setUp() {
jniUnderTest = new FlutterJNITest();
rendererUnderTest = new FlutterRenderer(jniUnderTest);
Copy link
Contributor

@matthew-carroll matthew-carroll Aug 15, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@matthew-carroll matthew-carroll Aug 15, 2019

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@xster xster Aug 20, 2019

Choose a reason for hiding this comment

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

  • 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.

}

@Test
public void itAllowsFirstFrameListenersToRemoveThemselvesInline() {
// --- Test Setup ---
AtomicInteger callbackCalled = new AtomicInteger(0);
OnFirstFrameRenderedListener callback = new OnFirstFrameRenderedListener() {
Copy link
Contributor

@matthew-carroll matthew-carroll Aug 15, 2019

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@xster xster Aug 20, 2019

Choose a reason for hiding this comment

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

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.

@Override
public void onFirstFrameRendered() {
callbackCalled.incrementAndGet();
rendererUnderTest.removeOnFirstFrameRenderedListener(this);
};
};
rendererUnderTest.addOnFirstFrameRenderedListener(callback);

// --- Execute Test ---
jniUnderTest.onFirstFrame();
Copy link
Contributor

@matthew-carroll matthew-carroll Aug 15, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@xster xster Aug 20, 2019

Choose a reason for hiding this comment

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

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)


// --- Verify Results ---
assertEquals(1, callbackCalled.get());

// --- Execute Test ---
// The callback removed itself from the listener list. A second call doesn't call the callback.
jniUnderTest.onFirstFrame();

// --- Verify Results ---
assertEquals(1, callbackCalled.get());
}

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

@matthew-carroll matthew-carroll Aug 15, 2019

Choose a reason for hiding this comment

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

Why is this here?

Copy link
Member Author

@xster xster Aug 20, 2019

Choose a reason for hiding this comment

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

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.

}
}
}
2 changes: 1 addition & 1 deletion testing/run_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ def EnsureDebugUnoptSkyPackagesAreBuilt():

def EnsureJavaTestsAreBuilt(android_out_dir):
ninja_command = [
'ninja',
'autoninja',
'-C',
android_out_dir,
'flutter/shell/platform/android:robolectric_tests'
Expand Down