Merge pull request #443 from vim-denops/fix-tests-flaky

Author: lambdalisue

tests/denops/runtime/functions/server/close_test.ts

@@ -217,19 +217,22 @@ testHost({
           "call denops#server#close()",
         ], "");
 
+        // Wait for times out
+        await delay(10);
+
         await host.call("execute", [
           "let g:__test_denops_server_status_after_close_called = denops#server#status()",
         ], "");
 
-        await t.step("closes the channel forcibly", async () => {
+        await t.step("closes the channel forcibly (flaky)", async () => {
           const actual = await host.call(
             "eval",
             "g:__test_denops_server_status_after_close_called",
           );
           assertEquals(actual, "stopped");
         });
 
-        await t.step("fires DenopsClosed", async () => {
+        await t.step("fires DenopsClosed (flaky)", async () => {
           assert(await host.call("exists", "g:__test_denops_closed_fired"));
         });
 

tests/denops/runtime/functions/server/start_test.ts

@@ -142,12 +142,30 @@ testHost({
         // 2nd call, do nothing.
         "let g:__test_denops_server_start_result_2nd = denops#server#start()",
         "let g:__test_denops_server_status_2nd = denops#server#status()",
-      ], "");
-      await host.call("execute", [
         // 3rd call with asynchronously, do nothing.
-        "let g:__test_denops_server_start_result_3rd = denops#server#start()",
-        "let g:__test_denops_server_status_3rd = denops#server#status()",
+        "function! TestDenopsServerStartAlreadyStarting(...) abort",
+        "  try",
+        "    let g:__test_denops_server_start_result_3rd = denops#server#start()",
+        "    let g:__test_denops_server_status_3rd = denops#server#status()",
+        "  catch",
+        "    let g:__test_denops_server_error_3rd = v:exception",
+        "  finally",
+        "    let g:__test_denops_server_called_3rd = 1",
+        "  endtry",
+        "endfunction",
+        "call timer_start(0, 'TestDenopsServerStartAlreadyStarting')",
       ], "");
+      // Wait for the 3rd call to finish.
+      await wait(
+        () => host.call("exists", "g:__test_denops_server_called_3rd"),
+        { message: "3rd call to start() is not finished" },
+      );
+
+      const error = await host.call(
+        "eval",
+        "get(g:, '__test_denops_server_error_3rd', '')",
+      );
+      assertEquals(error, "", "should not throws an error");
 
       await t.step("returns falsy", async () => {
         assertFalse(
@@ -158,13 +176,16 @@ testHost({
         );
       });
 
-      await t.step("does not change status from 'running'", async () => {
-        const actual = await host.call(
-          "eval",
-          "[g:__test_denops_server_status_2nd, g:__test_denops_server_status_3rd]",
-        );
-        assertEquals(actual, ["starting", "starting"]);
-      });
+      await t.step(
+        "does not change status from 'starting' (flaky)",
+        async () => {
+          const actual = await host.call(
+            "eval",
+            "[g:__test_denops_server_status_2nd, g:__test_denops_server_status_3rd]",
+          );
+          assertEquals(actual, ["starting", "starting"]);
+        },
+      );
     });
 
     await t.step("if already connected to shared-server", async (t) => {

tests/denops/runtime/functions/server/status_test.ts

@@ -30,6 +30,12 @@ testHost({
     await host.call("execute", [
       "call denops#server#start()",
       "let g:__test_denops_server_status_when_start_called = denops#server#status()",
+      "function! TestDenopsServerStatusBeforeReady(...) abort",
+      "  if exists('g:__test_denops_ready_fired') | return 0 | endif",
+      "  let g:__test_denops_server_status_before_ready = denops#server#status()",
+      "  call timer_start(0, 'TestDenopsServerStatusBeforeReady')",
+      "endfunction",
+      "call TestDenopsServerStatusBeforeReady()",
     ], "");
 
     await t.step(
@@ -46,14 +52,10 @@ testHost({
     await t.step(
       "returns 'preparing' before DenopsReady is fired (flaky)",
       async () => {
-        const actual = await wait(() =>
-          host.call(
-            "eval",
-            "exists('g:__test_denops_ready_fired')" +
-              "? 'DenopsReady is fired (flaky result)'" +
-              ": denops#server#status() !=# 'starting'" +
-              "  ? denops#server#status() : 0",
-          )
+        await wait(() => host.call("exists", "g:__test_denops_ready_fired"));
+        const actual = await host.call(
+          "eval",
+          "g:__test_denops_server_status_before_ready",
         );
         assertEquals(actual, "preparing");
       },

tests/denops/testutil/shared_server.ts

@@ -82,11 +82,7 @@ export async function useSharedServer(
   addrPromise.finally(() => addrReader.cancel());
 
   const abort = async (reason: unknown) => {
-    try {
-      aborter.abort(reason);
-    } catch {
-      // Already exited, do nothing.
-    }
+    aborter.abort(reason);
     await proc.status;
     await Promise.allSettled([
       proc.stdout.cancel(reason),
@@ -96,12 +92,13 @@ export async function useSharedServer(
 
   try {
     const addr = await deadline(addrPromise, timeout);
-    assert(typeof addr === "string");
-    const [_, host, port] = addr.match(/^([^:]*):(\d+)(?:\n|$)/) ?? [];
+    assert(addr, "Shared server did not returns an address");
+    const [_, host, portStr] = addr.match(/^([^:]*):(\d+)(?:\n|$)/) ?? [];
+    const port = Number.parseInt(portStr);
     return {
       addr: {
         host,
-        port: Number.parseInt(port),
+        port,
         toString: () => `${host}:${port}`,
       },
       stdout: stdoutReader2,

tests/denops/testutil/shared_server_test.ts

@@ -6,6 +6,7 @@ import {
   assertRejects,
 } from "jsr:@std/assert@^1.0.1";
 import { delay } from "jsr:@std/async@^1.0.1/delay";
+import { retry } from "jsr:@std/async@^1.0.1/retry";
 import { resolveTestDataPath } from "/denops-testdata/resolve.ts";
 import { useSharedServer } from "./shared_server.ts";
 
@@ -104,13 +105,15 @@ Deno.test("useSharedServer()", async (t) => {
     });
   });
 
-  await t.step("closes child process when rejectes", async () => {
-    await assertRejects(
-      async () => {
-        await useSharedServer({ timeout: 0 });
-      },
-      Error,
-      "Signal timed out",
-    );
+  await t.step("rejects if the server startup times out (flaky)", async () => {
+    await retry(async () => {
+      await assertRejects(
+        async () => {
+          await using _server = await useSharedServer({ timeout: 0 });
+        },
+        Error,
+        "Signal timed out",
+      );
+    });
   });
 });

tests/denops/testutil/wait.ts

@@ -1,4 +1,6 @@
 import { AssertionError } from "jsr:@std/assert@^1.0.1/assertion-error";
+import { abortable } from "jsr:@std/async@^1.0.1/abortable";
+import { delay } from "jsr:@std/async@^1.0.1/delay";
 import { getConfig } from "./conf.ts";
 
 const DEFAULT_TIMEOUT = 30_000;
@@ -32,40 +34,25 @@ export async function wait<T>(
     interval = DEFAULT_INTERVAL,
     message,
   } = { ...getConfig(), ...options };
-  const TIMEOUT = {};
+  const signal = AbortSignal.timeout(timeout);
 
-  let timeoutId: number | undefined;
-  const timeoutPromise = new Promise<never>((_, reject) => {
-    timeoutId = setTimeout(() => reject(TIMEOUT), timeout);
-  });
-
-  let intervalId: number | undefined;
-  const delay = () =>
-    new Promise<void>((resolve) => {
-      intervalId = setTimeout(resolve, interval);
-    });
+  const waitTruthy = async () => {
+    for (;;) {
+      const res = await fn();
+      if (res) {
+        return res;
+      }
+      await delay(interval, { signal });
+    }
+  };
 
   try {
-    return await Promise.race([
-      (async () => {
-        for (;;) {
-          const res = await fn();
-          if (res) {
-            return res;
-          }
-          await delay();
-        }
-      })(),
-      timeoutPromise,
-    ]);
+    return await abortable(waitTruthy(), signal);
   } catch (e) {
-    if (e === TIMEOUT) {
+    if (e === signal.reason) {
       const suffix = message ? `: ${message}` : ".";
       throw new AssertionError(`Timeout in ${timeout} millisec${suffix}`);
     }
     throw e;
-  } finally {
-    clearTimeout(timeoutId);
-    clearTimeout(intervalId);
   }
 }