Fix uncaught test failures

Prior to this commit, if you run `yarn unit cli/load`,
you would see a lot of unhandled promise rejection warnings related to
the fact that load calls a `saveTimestamps` function which expects the
SourceCred directory to contain the results of really loading SourceCred
plugins. However, when testing, these functions have been mocked, and so
saveTimestamps rejects. This rejection was not caught, and polluted the
test output without actually failing the tests.

This commit updates the tests so that saveTimestamps can be mocked (via
dependency injection) and we can both verify that it is invoked
correctly, and not pollute the test output with spurious warnings.

Test plan:
- `yarn test --full` passes
- `yarn unit cli/load` produces far fewer
UnhandledPromiseRejectionWarnings (there is still one unrelated one)
- loading sourcecred/research still works (as a canary)

Note: the PR which introduced this issue is #1162.

src/cli/load.js

@@ -172,60 +172,77 @@ export function makeLoadCommand(
   };
 }
 
-export const loadDefaultPlugins = async (options: LoadOptions) => {
+export type SaveTimestamps = (
-  const sourcecredCommand = (args) => [
+  $ReadOnlyArray<IBackendAdapterLoader>,
-    process.execPath,
+  RepoId
-    "--max_old_space_size=8192",
+) => Promise<void>;
-    process.argv[1],
-    ...args,
-  ];
-  const tasks = [
-    ...Common.defaultPlugins().map((pluginName) => ({
-      id: `load-${pluginName}`,
-      cmd: sourcecredCommand([
-        "load",
-        ...options.repoIds.map((repoId) => repoIdToString(repoId)),
-        "--output",
-        repoIdToString(options.output),
-        "--plugin",
-        pluginName,
-      ]),
-      deps: [],
-    })),
-  ];
 
-  const {success: loadSuccess} = await execDependencyGraph(tasks, {
+/**
-    taskPassLabel: "DONE",
+ * A wrapper around the default plugin loader.
-  });
+ *
-  if (!loadSuccess) {
+ * We wrap so that we can dependency inject saveTimestamps for testability.
-    throw new Error("Load tasks failed.");
+ * (saveTimestamps depends on the directory state that's generated by running
-  }
+ * load.)
-  addToRepoIdRegistry(options.output);
+ */
-  saveTimestamps(defaultAdapterLoaders(), options.output);
+export const makeLoadDefaultPlugins = (saveTimestamps: SaveTimestamps) => {
-  // HACK: Logically, we should have the PagerankTask be included in the
+  return async (options: LoadOptions) => {
-  // first execDependencyGraph run, depending on the other tasks completing.
+    const sourcecredCommand = (args) => [
-  //
+      process.execPath,
-  // However, running pagerank depends on loading the graph
+      "--max_old_space_size=8192",
-  // (analysis/loadGraph), which depends on the relevant repo being present
+      process.argv[1],
-  // in the RepoIdRegistry. And it is only in the RepoIdRegistry after the
+      ...args,
-  // call to execDependencyGraph has been successful.
+    ];
-  //
+    const tasks = [
-  // As a simple hack, we just call execDependencyGraph again with the
+      ...Common.defaultPlugins().map((pluginName) => ({
-  // pagerank command after the first one has been successful. This does have
+        id: `load-${pluginName}`,
-  // the awkward effect that CLI users will see two blocks of "task: SUCCESS"
+        cmd: sourcecredCommand([
-  // information from execDependencyGraph.
+          "load",
-  const pagerankTask = {
+          ...options.repoIds.map((repoId) => repoIdToString(repoId)),
-    id: "run-pagerank",
+          "--output",
-    cmd: sourcecredCommand(["pagerank", repoIdToString(options.output)]),
+          repoIdToString(options.output),
-    deps: [],
+          "--plugin",
+          pluginName,
+        ]),
+        deps: [],
+      })),
+    ];
+
+    const {success: loadSuccess} = await execDependencyGraph(tasks, {
+      taskPassLabel: "DONE",
+    });
+    if (!loadSuccess) {
+      throw new Error("Load tasks failed.");
+    }
+    addToRepoIdRegistry(options.output);
+    await saveTimestamps(defaultAdapterLoaders(), options.output);
+    // HACK: Logically, we should have the PagerankTask be included in the
+    // first execDependencyGraph run, depending on the other tasks completing.
+    //
+    // However, running pagerank depends on loading the graph
+    // (analysis/loadGraph), which depends on the relevant repo being present
+    // in the RepoIdRegistry. And it is only in the RepoIdRegistry after the
+    // call to execDependencyGraph has been successful.
+    //
+    // As a simple hack, we just call execDependencyGraph again with the
+    // pagerank command after the first one has been successful. This does have
+    // the awkward effect that CLI users will see two blocks of "task: SUCCESS"
+    // information from execDependencyGraph.
+    const pagerankTask = {
+      id: "run-pagerank",
+      cmd: sourcecredCommand(["pagerank", repoIdToString(options.output)]),
+      deps: [],
+    };
+    const {success: pagerankSuccess} = await execDependencyGraph(
+      [pagerankTask],
+      {
+        taskPassLabel: "DONE",
+      }
+    );
+    if (!pagerankSuccess) {
+      throw new Error("Pagerank task failed.");
+    }
+    return;
   };
-  const {success: pagerankSuccess} = await execDependencyGraph([pagerankTask], {
-    taskPassLabel: "DONE",
-  });
-  if (!pagerankSuccess) {
-    throw new Error("Pagerank task failed.");
-  }
-  return;
 };
 
 export const loadIndividualPlugin = async (
@@ -270,7 +287,15 @@ function addToRepoIdRegistry(repoId) {
   RepoIdRegistry.writeRegistry(newRegistry, Common.sourcecredDirectory());
 }
 
-async function saveTimestamps(
+/**
+ * Load timestamp data for every node in the serialized graph,
+ * then write the timestamps to disk.
+ *
+ * Note: This function calls well-tested code, but is not itself tested, except
+ * by the sharness load snapshot test.
+ * Modify with caution.
+ */
+export async function saveTimestamps(
   adapterLoaders: $ReadOnlyArray<IBackendAdapterLoader>,
   repoId: RepoId
 ) {
@@ -302,6 +327,7 @@ export const help: Command = async (args, std) => {
   }
 };
 
+const loadDefaultPlugins = makeLoadDefaultPlugins(saveTimestamps);
 const load = makeLoadCommand(loadIndividualPlugin, loadDefaultPlugins);
 
 export default load;

src/cli/load.test.js

@@ -6,13 +6,14 @@ import tmp from "tmp";
 import {run} from "./testUtil";
 import {
   makeLoadCommand,
-  loadDefaultPlugins,
+  makeLoadDefaultPlugins,
   loadIndividualPlugin,
   help,
 } from "./load";
 
 import * as RepoIdRegistry from "../core/repoIdRegistry";
 import {makeRepoId} from "../core/repoId";
+import {defaultAdapterLoaders} from "./pagerank";
 
 jest.mock("../tools/execDependencyGraph", () => jest.fn());
 jest.mock("../plugins/github/loadGithubData", () => ({
@@ -462,6 +463,7 @@ describe("cli/load", () => {
     const fooCombined = makeRepoId("foo", "combined");
     const fooBar = makeRepoId("foo", "bar");
     const fooBaz = makeRepoId("foo", "baz");
+    const loadDefaultPlugins = makeLoadDefaultPlugins(jest.fn());
 
     it("creates a load sub-task per plugin", async () => {
       execDependencyGraph.mockResolvedValue({success: true});
@@ -533,6 +535,21 @@ describe("cli/load", () => {
       expect(RepoIdRegistry.getRegistry(directory)).toEqual(expectedRegistry);
     });
 
+    it("calls saveTimestamps on success", async () => {
+      const saveTimestamps = jest.fn();
+      const loadDefaultPlugins = makeLoadDefaultPlugins(saveTimestamps);
+      execDependencyGraph.mockResolvedValue({success: true});
+      await loadDefaultPlugins({
+        output: fooCombined,
+        repoIds: [fooBar, fooBaz],
+      });
+      expect(saveTimestamps).toHaveBeenCalledTimes(1);
+      expect(saveTimestamps).toHaveBeenCalledWith(
+        defaultAdapterLoaders(),
+        fooCombined
+      );
+    });
+
     it("throws an load error on first execDependencyGraph failure", async () => {
       execDependencyGraph.mockResolvedValueOnce({success: false});
       const result = loadDefaultPlugins({