fix Julio's comments

Author: leotianlizhan

firebase-perf/src/test/java/com/google/firebase/perf/application/FrameMetricsRecorderTest.java

@@ -18,8 +18,6 @@
 import static com.google.common.truth.Truth.assertThat;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.doReturn;
-import static org.mockito.Mockito.times;
-import static org.mockito.Mockito.verify;
 import static org.mockito.MockitoAnnotations.initMocks;
 
 import android.app.Activity;
@@ -31,11 +29,9 @@
 import com.google.firebase.perf.metrics.FrameMetricsCalculator.PerfFrameMetrics;
 import com.google.firebase.perf.util.Optional;
 import java.util.HashMap;
-import java.util.List;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.mockito.ArgumentCaptor;
 import org.mockito.Mock;
 import org.robolectric.Robolectric;
 import org.robolectric.RobolectricTestRunner;
@@ -61,56 +57,39 @@ public void setUp() {
     stubFrameMetricsAggregatorData(fma, frameTimesDefault);
   }
 
-  /** FrameMetricsAggregator misuse prevention tests. Only 1 Activity per FMA. */
   @Test
-  public void frameMetricsAggregator_shouldOnlyUseTheSameSingleActivity() {
-    ArgumentCaptor<Activity> activityCaptor = ArgumentCaptor.forClass(Activity.class);
-    recorder.start();
-    recorder.stop();
+  public void stop_whileNotStarted_returnsEmptyResult() {
+    // nothing ever started
+    Optional<PerfFrameMetrics> result = recorder.stop();
+    assertThat(result.isAvailable()).isFalse();
+    // previous recording ended but no one has not started
     recorder.start();
     recorder.stop();
-    verify(fma, times(2)).add(activityCaptor.capture());
-    verify(fma, times(2)).remove(activityCaptor.capture());
-    List<Activity> activities = activityCaptor.getAllValues();
-    assertThat(activities.stream().allMatch(a -> a == activity)).isTrue();
-  }
-
-  @Test
-  public void start_whileAlreadyStarted_doesNotCallFMATwice() {
-    recorder.start();
-    recorder.start();
-    verify(fma, times(1)).add(any());
+    Optional<PerfFrameMetrics> result2 = recorder.stop();
+    assertThat(result2.isAvailable()).isFalse();
   }
 
   @Test
-  public void start_afterPreviousEnded_doesCallFMASuccessfully() {
+  public void startAndStop_calledTwice_ignoresSecondCall() {
     recorder.start();
-    recorder.stop();
-    recorder.start();
-    verify(fma, times(2)).add(any());
-  }
+    recorder.start(); // 2nd call is ignored
+    stubFrameMetricsAggregatorData(fma, new int[][] {{1, 1}, {17, 3}, {800, 1}});
+    Optional<PerfFrameMetrics> result1 = recorder.stop();
+    stubFrameMetricsAggregatorData(fma, frameTimes2); // 2nd call is ignored
+    Optional<PerfFrameMetrics> result2 = recorder.stop();
 
-  @Test
-  public void stop_whileNotStarted_returnsEmptyResult() {
-    Optional<PerfFrameMetrics> result;
-    result = recorder.stop();
-    assertThat(result.isAvailable()).isFalse();
-  }
+    assertThat(result1.isAvailable()).isTrue();
+    assertThat(result2.isAvailable()).isFalse();
 
-  @Test
-  public void stop_calledTwice_returnsEmptyResult() {
-    Optional<PerfFrameMetrics> result;
-    recorder.start();
-    recorder.stop();
-    result = recorder.stop();
-    assertThat(result.isAvailable()).isFalse();
+    assertThat(result1.get().getTotalFrames()).isEqualTo(1 + 3 + 1);
+    assertThat(result1.get().getTotalFrames()).isEqualTo(3 + 1);
+    assertThat(result1.get().getTotalFrames()).isEqualTo(1);
   }
 
   @Test
   public void startAndStop_calledInCorrectOrder_returnsValidResult() {
-    Optional<PerfFrameMetrics> result;
     recorder.start();
-    result = recorder.stop();
+    Optional<PerfFrameMetrics> result = recorder.stop();
     assertThat(result.isAvailable()).isTrue();
     assertThat(result.get().getTotalFrames()).isEqualTo(3);
     assertThat(result.get().getSlowFrames()).isEqualTo(2);
@@ -120,76 +99,74 @@ public void startAndStop_calledInCorrectOrder_returnsValidResult() {
   @Test
   public void startAndStopSubTrace_activityRecordingNeverStarted_returnsEmptyResult() {
     Fragment fragment = new Fragment();
-    Optional<PerfFrameMetrics> result;
 
     stubFrameMetricsAggregatorData(fma, frameTimes1);
     recorder.startSubTrace(fragment);
     stubFrameMetricsAggregatorData(fma, frameTimes2);
-    result = recorder.stopSubTrace(fragment);
+    Optional<PerfFrameMetrics> result = recorder.stopSubTrace(fragment);
     assertThat(result.isAvailable()).isFalse();
   }
 
   @Test
   public void startAndStopSubTrace_activityRecordingHasEnded_returnsEmptyResult() {
     Fragment fragment = new Fragment();
-    Optional<PerfFrameMetrics> result;
 
     recorder.start();
     recorder.stop();
     stubFrameMetricsAggregatorData(fma, frameTimes1);
     recorder.startSubTrace(fragment);
     stubFrameMetricsAggregatorData(fma, frameTimes2);
-    result = recorder.stopSubTrace(fragment);
+    Optional<PerfFrameMetrics> result = recorder.stopSubTrace(fragment);
     assertThat(result.isAvailable()).isFalse();
   }
 
+  /**
+   * This scenario happens only when we hook on to the same instance of fragment twice, which should
+   * never happen
+   */
   @Test
   public void startAndStopSubTrace_whenCalledTwice_ignoresSecondCall() {
     Fragment fragment = new Fragment();
-    Optional<PerfFrameMetrics> result1;
-    Optional<PerfFrameMetrics> result2;
 
-    // Happens only when we hook on to the same instance of fragment twice. Very unlikely.
+    // comments are in this format: total frames, slow frames, frozen frames
     recorder.start();
     stubFrameMetricsAggregatorData(fma, new int[][] {{1, 1}, {17, 1}, {800, 1}}); // 3, 2, 1
     recorder.startSubTrace(fragment);
-    stubFrameMetricsAggregatorData(fma, new int[][] {{1, 2}, {17, 3}, {800, 2}}); // 7, 5, 2
-    recorder.startSubTrace(fragment);
+    stubFrameMetricsAggregatorData(fma, new int[][] {{1, 2}, {17, 3}, {800, 2}}); // ignored
+    recorder.startSubTrace(fragment); // this call is ignored
     stubFrameMetricsAggregatorData(fma, new int[][] {{1, 5}, {17, 4}, {800, 5}}); // 14, 9, 5
-    result1 = recorder.stopSubTrace(fragment);
-    stubFrameMetricsAggregatorData(fma, new int[][] {{1, 6}, {17, 5}, {800, 5}}); // 16, 10, 5
-    result2 = recorder.stopSubTrace(fragment);
+    Optional<PerfFrameMetrics> result1 = recorder.stopSubTrace(fragment);
+    stubFrameMetricsAggregatorData(fma, new int[][] {{1, 6}, {17, 5}, {800, 5}}); // ignored
+    Optional<PerfFrameMetrics> result2 = recorder.stopSubTrace(fragment); // this call is ignored
 
     // total = 14 - 3 = 11, slow = 9 - 2 = 7, frozen = 5 - 1 = 4
-    assertThat(result1.get().getTotalFrames()).isEqualTo(11);
-    assertThat(result1.get().getSlowFrames()).isEqualTo(7);
-    assertThat(result1.get().getFrozenFrames()).isEqualTo(4);
+    assertThat(result1.get().getTotalFrames()).isEqualTo(14 - 3);
+    assertThat(result1.get().getSlowFrames()).isEqualTo(9 - 2);
+    assertThat(result1.get().getFrozenFrames()).isEqualTo(5 - 1);
     assertThat(result2.isAvailable()).isFalse();
   }
 
   @Test
   public void stopAndStopSubTrace_whenNoSubTraceWithGivenKeyExists_returnsEmptyResult() {
     Fragment fragment1 = new Fragment();
     Fragment fragment2 = new Fragment();
-    Optional<PerfFrameMetrics> result;
 
     recorder.start();
 
     recorder.startSubTrace(fragment1);
-    result = recorder.stopSubTrace(fragment2);
+    Optional<PerfFrameMetrics> result = recorder.stopSubTrace(fragment2);
 
     assertThat(result.isAvailable()).isFalse();
   }
 
   @Test
   public void startAndStopSubTrace_duringActivityRecording_returnsValidResult() {
     Fragment fragment = new Fragment();
-    Optional<PerfFrameMetrics> result;
     recorder.start();
     stubFrameMetricsAggregatorData(fma, frameTimes1);
     recorder.startSubTrace(fragment);
     stubFrameMetricsAggregatorData(fma, frameTimes2);
-    result = recorder.stopSubTrace(fragment);
+    Optional<PerfFrameMetrics> result = recorder.stopSubTrace(fragment);
     assertThat(result.isAvailable()).isTrue();
     // frameTimes2 - frameTimes1
     assertThat(result.get().getTotalFrames()).isEqualTo(9);
@@ -201,26 +178,25 @@ public void startAndStopSubTrace_duringActivityRecording_returnsValidResult() {
   public void startAndStopSubTrace_whenTwoSubTracesOverlap_returnsCorrectResults() {
     Fragment fragment1 = new Fragment();
     Fragment fragment2 = new Fragment();
-    Optional<PerfFrameMetrics> subTrace1;
-    Optional<PerfFrameMetrics> subTrace2;
     recorder.start();
+    // comments are in this format: total frames, slow frames, frozen frames
     stubFrameMetricsAggregatorData(fma, new int[][] {{1, 1}, {17, 1}, {800, 1}}); // 3, 2, 1
     recorder.startSubTrace(fragment1);
     stubFrameMetricsAggregatorData(fma, new int[][] {{1, 2}, {17, 3}, {800, 2}}); // 7, 5, 2
     recorder.startSubTrace(fragment2);
     stubFrameMetricsAggregatorData(fma, new int[][] {{1, 5}, {17, 4}, {800, 5}}); // 14, 9, 5
-    subTrace1 = recorder.stopSubTrace(fragment1);
+    Optional<PerfFrameMetrics> subTrace1 = recorder.stopSubTrace(fragment1);
     stubFrameMetricsAggregatorData(fma, new int[][] {{1, 6}, {17, 5}, {800, 5}}); // 16, 10, 5
-    subTrace2 = recorder.stopSubTrace(fragment2);
-
-    // total = 14 - 3 = 11, slow = 9 - 2 = 7, frozen = 5 - 1 = 4
-    assertThat(subTrace1.get().getTotalFrames()).isEqualTo(11);
-    assertThat(subTrace1.get().getSlowFrames()).isEqualTo(7);
-    assertThat(subTrace1.get().getFrozenFrames()).isEqualTo(4);
-    // total = 16 - 7 = 9, slow = 10 - 5 = 5, frozen = 5 - 2 = 3
-    assertThat(subTrace2.get().getTotalFrames()).isEqualTo(9);
-    assertThat(subTrace2.get().getSlowFrames()).isEqualTo(5);
-    assertThat(subTrace2.get().getFrozenFrames()).isEqualTo(3);
+    Optional<PerfFrameMetrics> subTrace2 = recorder.stopSubTrace(fragment2);
+
+    // 3rd snapshot - 1st snapshot
+    assertThat(subTrace1.get().getTotalFrames()).isEqualTo(14 - 3);
+    assertThat(subTrace1.get().getSlowFrames()).isEqualTo(9 - 2);
+    assertThat(subTrace1.get().getFrozenFrames()).isEqualTo(5 - 1);
+    // 4th snapshot - 2nd snapshot
+    assertThat(subTrace2.get().getTotalFrames()).isEqualTo(16 - 7);
+    assertThat(subTrace2.get().getSlowFrames()).isEqualTo(10 - 5);
+    assertThat(subTrace2.get().getFrozenFrames()).isEqualTo(5 - 2);
   }
 
   private static Activity createFakeActivity(boolean isHardwareAccelerated) {