fix(desktop): fix time limit checkbox (#19)

* test(desktop): send empty measure to reflect reality

* test(desktop): go for maximum coverage

Also add a failing test for time limit checkbox bug

* fix(desktop): fix time limit checbox

Measures were stopped even if the checkbox was unchecked

* chore(desktop): bump version

Author: Almouro

flipper-desktop/package.json

@@ -2,7 +2,7 @@
   "$schema": "https://fbflipper.com/schemas/plugin-package/v2.json",
   "name": "flipper-plugin-rn-perf-monitor",
   "id": "rn-perf-monitor",
-  "version": "0.4.0",
+  "version": "0.4.1",
   "main": "dist/bundle.js",
   "flipperBundlerEntry": "src/index.tsx",
   "license": "MIT",

flipper-desktop/src/PerfMonitorView.tsx

@@ -54,6 +54,9 @@ const TimeLimitControl = ({
     <FormControlLabel
       control={
         <Checkbox
+          inputProps={{
+            "aria-label": "Time limit enabled",
+          }}
           checked={timeLimitEnabled}
           onChange={(event: { target: { checked: boolean } }) => {
             toggleTimeLimit(event.target.checked);
@@ -63,6 +66,9 @@ const TimeLimitControl = ({
       label="Enable time limit of "
     />
     <TextField
+      inputProps={{
+        "aria-label": "Time limit",
+      }}
       type="number"
       onChange={({ target: { value } }) =>
         setTimeLimit(Math.floor(parseInt(value, 10)))
@@ -96,7 +102,11 @@ export const PerfMonitorView = ({
   const [timeLimitEnabled, setTimeLimitEnabled] = useState(true);
   const [timeLimit, setTimeLimit] = useState<number | null>(DEFAULT_TIME_LIMIT);
   useEffect(() => {
-    if (timeLimit && measures.length > timeLimit / MEASURE_INTERVAL) {
+    if (
+      timeLimitEnabled &&
+      timeLimit &&
+      measures.length > timeLimit / MEASURE_INTERVAL
+    ) {
       stopMeasuring();
     }
   }, [timeLimit, measures]);

flipper-desktop/src/__tests__/Plugin.spec.tsx

@@ -11,48 +11,96 @@ require("@testing-library/react");
 jest.mock("react-apexcharts", () => "apex-charts");
 jest.mock("apexcharts", () => ({ exec: jest.fn() }));
 
-test("displays FPS data and scoring", async () => {
-  const { renderer, sendEvent, onSend } = TestUtils.renderPlugin(Plugin);
-
-  fireEvent.click(renderer.getByText("Start Measuring"));
-  expect(onSend).toHaveBeenCalledWith("startMeasuring", undefined);
+const setupPlugin = () => {
+  const { instance, renderer, sendEvent, onSend } =
+    TestUtils.renderPlugin(Plugin);
 
-  sendEvent("addRecord", {
-    JS: 30,
-    UI: 25,
-    expected: 30,
-  });
+  // First measure on Android is always 0 and is ignored by plugin
   sendEvent("addRecord", {
     JS: 0,
-    UI: 30,
+    UI: 0,
     expected: 30,
   });
 
+  return {
+    addMeasure: ({ JS, UI }: { JS: number; UI: number }) =>
+      sendEvent("addRecord", {
+        JS,
+        UI,
+        expected: 30,
+      }),
+    clickStart: () => fireEvent.click(renderer.getByText("Start Measuring")),
+    clickStop: () => fireEvent.click(renderer.getByText("Stop Measuring")),
+    expectToMatchSnapshot: () => {
+      expect(
+        (renderer.baseElement as HTMLBodyElement).textContent
+      ).toMatchSnapshot();
+      expect(renderer.baseElement).toMatchSnapshot();
+    },
+    setTimeLimit: (limit: number) => {
+      const input = renderer.getByLabelText("Time limit");
+      fireEvent.change(input, { target: { value: limit } });
+    },
+    clickTimeLimitCheckbox: () => {
+      const checkbox = renderer.getByLabelText("Time limit enabled");
+      fireEvent.click(checkbox);
+    },
+
+    renderer,
+    instance,
+    onSend,
+  };
+};
+
+test("displays FPS data and scoring", async () => {
+  const { addMeasure, expectToMatchSnapshot } = setupPlugin();
+
+  addMeasure({ JS: 30, UI: 25 });
+  addMeasure({ JS: 0, UI: 30 });
+
+  expectToMatchSnapshot();
+});
+
+test("clicking start should reset measures and start measures", () => {
+  const { addMeasure, clickStart, clickStop, instance, onSend } = setupPlugin();
+
+  addMeasure({ JS: 30, UI: 25 });
+  addMeasure({ JS: 0, UI: 30 });
+
+  clickStart();
+  expect(onSend).toHaveBeenCalledWith("startMeasuring", undefined);
   onSend.mockClear();
 
-  fireEvent.click(renderer.getByText("Stop Measuring"));
+  expect(instance.measures.get()).toEqual([]);
+
+  clickStop();
   expect(onSend).toHaveBeenCalledWith("stopMeasuring", undefined);
+});
 
-  expect(
-    (renderer.baseElement as HTMLBodyElement).textContent
-  ).toMatchSnapshot();
-  expect(renderer.baseElement).toMatchSnapshot();
+test("stops after set time limit", () => {
+  const { addMeasure, setTimeLimit, onSend } = setupPlugin();
+
+  setTimeLimit(1000);
+
+  addMeasure({ JS: 30, UI: 30 });
+  addMeasure({ JS: 30, UI: 30 });
+  expect(onSend).not.toHaveBeenCalledWith("stopMeasuring", undefined);
+  // 1000ms means 3 measures would be displayed: 0, 500 and 1000
+  addMeasure({ JS: 30, UI: 30 });
+  expect(onSend).toHaveBeenCalledWith("stopMeasuring", undefined);
 });
 
-test("clicking start should reset measures", () => {
-  const { renderer, sendEvent, instance } = TestUtils.renderPlugin(Plugin);
+test("continues after set time limit if time limit disabled", () => {
+  const { addMeasure, clickTimeLimitCheckbox, setTimeLimit, onSend } =
+    setupPlugin();
 
-  sendEvent("addRecord", {
-    JS: 30,
-    UI: 25,
-    expected: 30,
-  });
-  sendEvent("addRecord", {
-    JS: 0,
-    UI: 30,
-    expected: 30,
-  });
+  setTimeLimit(1000);
+  clickTimeLimitCheckbox();
 
-  fireEvent.click(renderer.getByText("Start Measuring"));
-  expect(instance.measures.get()).toEqual([]);
+  addMeasure({ JS: 30, UI: 30 });
+  addMeasure({ JS: 30, UI: 30 });
+  expect(onSend).not.toHaveBeenCalledWith("stopMeasuring", undefined);
+  // 1000ms means 3 measures would be displayed: 0, 500 and 1000
+  addMeasure({ JS: 30, UI: 30 });
+  expect(onSend).not.toHaveBeenCalledWith("stopMeasuring", undefined);
 });

flipper-desktop/src/__tests__/__snapshots__/Plugin.spec.tsx.snap

@@ -1,6 +1,6 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`displays FPS data and scoring 1`] = `"Performance0Average JS FPS0Average UI FPS60JS  threadlock0.506s (100%)Start MeasuringEnable time limit of ms"`;
+exports[`displays FPS data and scoring 1`] = `"Performance35Average JS FPS30Average UI FPS55JS  threadlock0.506s (50%)Start MeasuringEnable time limit of ms"`;
 
 exports[`displays FPS data and scoring 2`] = `
 <body>
@@ -21,7 +21,7 @@ exports[`displays FPS data and scoring 2`] = `
           class="MuiBox-root MuiBox-root-1"
         >
           <div
-            aria-valuenow="0"
+            aria-valuenow="35"
             class="MuiCircularProgress-root MuiCircularProgress-colorPrimary MuiCircularProgress-determinate"
             role="progressbar"
             style="width: 80px; height: 80px; transform: rotate(-90deg); color: rgb(255, 65, 54);"
@@ -37,7 +37,7 @@ exports[`displays FPS data and scoring 2`] = `
                 fill="none"
                 r="20.2"
                 stroke-width="3.6"
-                style="stroke-dasharray: 126.920; stroke-dashoffset: 126.920px;"
+                style="stroke-dasharray: 126.920; stroke-dashoffset: 82.498px;"
               />
             </svg>
           </div>
@@ -48,7 +48,7 @@ exports[`displays FPS data and scoring 2`] = `
               class="MuiTypography-root MuiTypography-caption MuiTypography-colorTextSecondary"
               style="font-size: 28px; color: rgb(255, 65, 54);"
             >
-              0
+              35
             </div>
           </div>
         </div>
@@ -73,7 +73,7 @@ exports[`displays FPS data and scoring 2`] = `
             <td
               class="MuiTableCell-root MuiTableCell-body MuiTableCell-alignRight"
             >
-              0
+              30
             </td>
           </tr>
           <tr
@@ -89,7 +89,7 @@ exports[`displays FPS data and scoring 2`] = `
             <td
               class="MuiTableCell-root MuiTableCell-body MuiTableCell-alignRight"
             >
-              60
+              55
             </td>
           </tr>
           <tr
@@ -105,7 +105,7 @@ exports[`displays FPS data and scoring 2`] = `
             <td
               class="MuiTableCell-root MuiTableCell-body MuiTableCell-alignRight"
             >
-              0.506s (100%)
+              0.506s (50%)
             </td>
           </tr>
         </tbody>
@@ -155,6 +155,7 @@ exports[`displays FPS data and scoring 2`] = `
                 class="MuiIconButton-label"
               >
                 <input
+                  aria-label="Time limit enabled"
                   checked=""
                   class="PrivateSwitchBase-input-6"
                   data-indeterminate="false"
@@ -190,6 +191,7 @@ exports[`displays FPS data and scoring 2`] = `
             >
               <input
                 aria-invalid="false"
+                aria-label="Time limit"
                 class="MuiInputBase-input MuiInput-input"
                 type="number"
                 value="10000"