Refactor loadGraph out of exportGraph (#1113)

This pulls the logic for loading a SourceCred graph from disk out
`cli/exportGraph` and into `analysis/loadGraph`. The rationale is that
`exportGraph` is not the only command that wants the ability to load a
graph from the analysis adapters.

The new command has a clean return signature that reports whether the
load was successful, or failed because the graph wasn't loaded, or
failed due to an error in plugin code.

Testing of the loading logic has been moved to `loadGraph.test`, and the
CLI has been refactored so that the loadGraph method is dependency
injected. This allows for (IMO) cleaner testing of the CLI method.

There is one (deliberate) change in behavior, which is that the command no
longer throws an error if no plugins are included; instead it will just
export an empty graph. I don't have a strong preference between the two
behaviors; changing it was just more convenient.

Test plan: New unit tests have been added, and tests of the cli command
have been re-written. As a sanity check, I've verified that the
following sequence still works:

```
$ yarn backend
$ node bin/sourcecred.js load sourcecred/pm
$ node bin/sourcecred.js export-graph sourcecred/pm
```

Nearly perfect code coverage is maintained. One line is uncovered, and
it's the line that injects in the actual graph loading behavior.

src/analysis/loadGraph.js

@@ -0,0 +1,69 @@
+// @flow
+
+import {Graph} from "../core/graph";
+import * as NullUtil from "../util/null";
+import * as RepoIdRegistry from "../core/repoIdRegistry";
+import {type RepoId} from "../core/repoId";
+
+import type {IAnalysisAdapter} from "./analysisAdapter";
+
+/**
+ * Module for loading a graph from a SOURCECRED_DIRECTORY.
+ */
+
+export type LoadGraphResult =
+  | {|+status: "SUCCESS", +graph: Graph|}
+  | {|+status: "REPO_NOT_LOADED"|}
+  | {|+status: "PLUGIN_FAILURE", +pluginName: string, +error: Error|};
+
+type GraphOrError =
+  | {|+type: "GRAPH", +graph: Graph|}
+  | {|+type: "ERROR", +pluginName: string, +error: Error|};
+
+/**
+ * Load a Graph from disk.
+ *
+ * Loads a combined graph by merging each adapter's graph.
+ * Before this can succeed, every plugin represented in the adapter array
+ * must have already downloaded and serialized data for the provided
+ * repoId, e.g. by calling `sourcecred load REPO_ID`.
+ */
+export async function loadGraph(
+  sourcecredDirectory: string,
+  adapters: $ReadOnlyArray<IAnalysisAdapter>,
+  repoId: RepoId
+): Promise<LoadGraphResult> {
+  const registry = RepoIdRegistry.getRegistry(sourcecredDirectory);
+  if (RepoIdRegistry.getEntry(registry, repoId) == null) {
+    return {status: "REPO_NOT_LOADED"};
+  }
+  async function graphForAdapter(
+    adapter: IAnalysisAdapter
+  ): Promise<GraphOrError> {
+    try {
+      const graph = await adapter.load(
+        sourcecredDirectory,
+        NullUtil.get(repoId)
+      );
+      return {type: "GRAPH", graph};
+    } catch (e) {
+      return {type: "ERROR", pluginName: adapter.declaration().name, error: e};
+    }
+  }
+  const results: GraphOrError[] = await Promise.all(
+    adapters.map(graphForAdapter)
+  );
+  const graphs: Graph[] = [];
+  for (const r of results) {
+    if (r.type === "ERROR") {
+      return {
+        status: "PLUGIN_FAILURE",
+        pluginName: r.pluginName,
+        error: r.error,
+      };
+    } else {
+      graphs.push(r.graph);
+    }
+  }
+  return {status: "SUCCESS", graph: Graph.merge(graphs)};
+}

src/analysis/loadGraph.test.js

@@ -0,0 +1,121 @@
+// @flow
+
+import tmp from "tmp";
+import path from "path";
+
+import {Graph, NodeAddress, EdgeAddress} from "../core/graph";
+import type {IAnalysisAdapter} from "../analysis/analysisAdapter";
+import * as RepoIdRegistry from "../core/repoIdRegistry";
+import {makeRepoId, type RepoId} from "../core/repoId";
+import {loadGraph} from "./loadGraph";
+
+class MockAnalysisAdapter implements IAnalysisAdapter {
+  _resolutionGraph: ?Graph;
+  _name: string;
+
+  /**
+   * Takes a name for the plugin, and a graph that
+   * is provided as a result of a successful load.
+   * If no graph is provided, then load will fail.
+   */
+  constructor(name: string, resolutionGraph: ?Graph) {
+    this._name = name;
+    this._resolutionGraph = resolutionGraph;
+  }
+
+  declaration() {
+    return {
+      name: this._name,
+      nodePrefix: NodeAddress.empty,
+      edgePrefix: EdgeAddress.empty,
+      nodeTypes: [],
+      edgeTypes: [],
+    };
+  }
+
+  async load(
+    _unused_sourcecredDirectory: string,
+    _unused_repoId: RepoId
+  ): Promise<Graph> {
+    if (this._resolutionGraph != null) {
+      return this._resolutionGraph;
+    } else {
+      throw new Error("MockAnalysisAdapterRejects");
+    }
+  }
+}
+
+describe("analysis/loadGraph", () => {
+  function setUpRegistryWithId(repoId: RepoId) {
+    const dirname = tmp.dirSync().name;
+    process.env.SOURCECRED_DIRECTORY = dirname;
+    const registry = RepoIdRegistry.addEntry(RepoIdRegistry.emptyRegistry(), {
+      repoId,
+    });
+    RepoIdRegistry.writeRegistry(registry, dirname);
+    return dirname;
+  }
+  describe("loadGraph", () => {
+    it("returns status:REPO_NOT_LOADED when sourcecred directory is empty", async () => {
+      const dirname = tmp.dirSync().name;
+      const result = await loadGraph(
+        dirname,
+        [new MockAnalysisAdapter("foo")],
+        makeRepoId("foo", "bar")
+      );
+      expect(result).toEqual({status: "REPO_NOT_LOADED"});
+    });
+    it("returns status:REPO_NOT_LOADED when sourcecred directory doesn't exist", async () => {
+      const dirname = path.join(tmp.dirSync().name, "nonexistent");
+      const result = await loadGraph(
+        dirname,
+        [new MockAnalysisAdapter("foo")],
+        makeRepoId("foo", "bar")
+      );
+      expect(result).toEqual({status: "REPO_NOT_LOADED"});
+    });
+    it("returns status:REPO_NOT_LOADED when the repo just isn't in the registry", async () => {
+      const dirname = setUpRegistryWithId(makeRepoId("zod", "zoink"));
+      const result = await loadGraph(
+        dirname,
+        [new MockAnalysisAdapter("foo")],
+        makeRepoId("foo", "bar")
+      );
+      expect(result).toEqual({status: "REPO_NOT_LOADED"});
+    });
+    it("returns status:SUCCESS with merged graph on success", async () => {
+      const g1 = new Graph().addNode(NodeAddress.fromParts(["g1"]));
+      const g2 = new Graph().addNode(NodeAddress.fromParts(["g2"]));
+      const m1 = new MockAnalysisAdapter("foo", g1);
+      const m2 = new MockAnalysisAdapter("bar", g2);
+      const mergedGraph = Graph.merge([g1, g2]);
+      const dir = setUpRegistryWithId(makeRepoId("foo", "bar"));
+      const result = await loadGraph(dir, [m1, m2], makeRepoId("foo", "bar"));
+      expect(result.status).toEqual("SUCCESS");
+      if (result.status !== "SUCCESS") {
+        throw new Error("Unreachable, needed to satisfy flow.");
+      }
+      expect(mergedGraph.equals(result.graph)).toBe(true);
+    });
+    it("returns an empty graph if no adapters provided", async () => {
+      const dir = setUpRegistryWithId(makeRepoId("foo", "bar"));
+      const result = await loadGraph(dir, [], makeRepoId("foo", "bar"));
+      expect(result.status).toEqual("SUCCESS");
+      if (result.status !== "SUCCESS") {
+        throw new Error("Unreachable, needed to satisfy flow.");
+      }
+      expect(result.graph.equals(new Graph())).toBe(true);
+    });
+    it("returns a status:PLUGIN_FAILURE if the plugin errors", async () => {
+      const mockAdapter = new MockAnalysisAdapter("bar");
+      const repoId = makeRepoId("foo", "bar");
+      const dir = setUpRegistryWithId(repoId);
+      const result = await loadGraph(dir, [mockAdapter], repoId);
+      expect(result).toEqual({
+        status: "PLUGIN_FAILURE",
+        pluginName: "bar",
+        error: new Error("MockAnalysisAdapterRejects"),
+      });
+    });
+  });
+});

src/cli/exportGraph.js

@@ -1,16 +1,13 @@
 // @flow
 // Implementation of `sourcecred export-graph`.
 
-import {Graph} from "../core/graph";
-import * as NullUtil from "../util/null";
-import * as RepoIdRegistry from "../core/repoIdRegistry";
 import {repoIdToString, stringToRepoId, type RepoId} from "../core/repoId";
 import dedent from "../util/dedent";
 import type {Command} from "./command";
 import * as Common from "./common";
 import stringify from "json-stable-stringify";
+import {loadGraph, type LoadGraphResult} from "../analysis/loadGraph";
 
-import type {IAnalysisAdapter} from "../analysis/analysisAdapter";
 import {AnalysisAdapter as GithubAnalysisAdapter} from "../plugins/github/analysisAdapter";
 import {AnalysisAdapter as GitAnalysisAdapter} from "../plugins/git/analysisAdapter";
 
@@ -52,15 +49,10 @@ function die(std, message) {
 }
 
 export function makeExportGraph(
-  adapters: $ReadOnlyArray<IAnalysisAdapter>
+  loader: (RepoId) => Promise<LoadGraphResult>
 ): Command {
   return async function exportGraph(args, std) {
     let repoId: RepoId | null = null;
-    if (adapters.length === 0) {
-      std.err("fatal: no plugins available");
-      std.err("fatal: this is likely a build error");
-      return 1;
-    }
     for (let i = 0; i < args.length; i++) {
       switch (args[i]) {
         case "--help": {
@@ -81,39 +73,40 @@ export function makeExportGraph(
       return die(std, "no repository ID provided");
     }
 
-    const directory = Common.sourcecredDirectory();
+    const result: LoadGraphResult = await loader(repoId);
-    const registry = RepoIdRegistry.getRegistry(directory);
+    switch (result.status) {
-    if (RepoIdRegistry.getEntry(registry, repoId) == null) {
+      case "REPO_NOT_LOADED": {
         const repoIdStr = repoIdToString(repoId);
         std.err(`fatal: repository ID ${repoIdStr} not loaded`);
         std.err(`Try running \`sourcecred load ${repoIdStr}\` first.`);
         return 1;
       }
-    async function graphForAdapter(adapter: IAnalysisAdapter): Promise<Graph> {
+      case "PLUGIN_FAILURE": {
-      try {
+        std.err(
-        return await adapter.load(directory, NullUtil.get(repoId));
+          `fatal: plugin "${result.pluginName}" errored: ${
-      } catch (e) {
+            result.error.message
-        throw new Error(
+          }`
-          `plugin "${adapter.declaration().name}" errored: ${e.message}`
         );
+        return 1;
       }
-    }
+      case "SUCCESS": {
-    let graphs: Graph[];
+        const graphJSON = result.graph.toJSON();
-    try {
-      graphs = await Promise.all(adapters.map(graphForAdapter));
-    } catch (e) {
-      return die(std, e.message);
-    }
-    const graph = Graph.merge(graphs);
-    const graphJSON = graph.toJSON();
         std.out(stringify(graphJSON));
         return 0;
+      }
+      // istanbul ignore next: unreachable per Flow
+      default: {
+        std.err(`Unexpected status: ${(result.status: empty)}`);
+        return 1;
+      }
+    }
   };
 }
 
 const defaultAdapters = [new GithubAnalysisAdapter(), new GitAnalysisAdapter()];
-
+const defaultLoadGraph = (r: RepoId) =>
-export const exportGraph = makeExportGraph(defaultAdapters);
+  loadGraph(Common.sourcecredDirectory(), defaultAdapters, r);
+export const exportGraph = makeExportGraph(defaultLoadGraph);
 
 export const help: Command = async (args, std) => {
   if (args.length === 0) {

src/cli/exportGraph.test.js

@@ -1,51 +1,11 @@
 // @flow
 
-import tmp from "tmp";
-
 import {run} from "./testUtil";
 import {help, makeExportGraph} from "./exportGraph";
-import {Graph, NodeAddress, EdgeAddress} from "../core/graph";
+import {Graph, NodeAddress} from "../core/graph";
-import type {IAnalysisAdapter} from "../analysis/analysisAdapter";
 import stringify from "json-stable-stringify";
 
-import * as RepoIdRegistry from "../core/repoIdRegistry";
+import {makeRepoId} from "../core/repoId";
-import {makeRepoId, type RepoId} from "../core/repoId";
-
-class MockAnalysisAdapter implements IAnalysisAdapter {
-  _resolutionGraph: ?Graph;
-  _name: string;
-
-  /**
-   * Takes a name for the plugin, and a graph that
-   * is provided as a result of a successful load.
-   * If no graph is provided, then load will fail.
-   */
-  constructor(name: string, resolutionGraph: ?Graph) {
-    this._name = name;
-    this._resolutionGraph = resolutionGraph;
-  }
-
-  declaration() {
-    return {
-      name: this._name,
-      nodePrefix: NodeAddress.empty,
-      edgePrefix: EdgeAddress.empty,
-      nodeTypes: [],
-      edgeTypes: [],
-    };
-  }
-
-  async load(
-    _unused_sourcecredDirectory: string,
-    _unused_repoId: RepoId
-  ): Promise<Graph> {
-    if (this._resolutionGraph != null) {
-      return this._resolutionGraph;
-    } else {
-      throw new Error("MockAnalysisAdapterRejects");
-    }
-  }
-}
 
 describe("cli/exportGraph", () => {
   describe("'help' command", () => {
@@ -70,18 +30,8 @@ describe("cli/exportGraph", () => {
   });
 
   describe("'exportGraph' command", () => {
-    function setUpRegistryWithId(repoId: RepoId) {
-      const dirname = tmp.dirSync().name;
-      process.env.SOURCECRED_DIRECTORY = dirname;
-      const registry = RepoIdRegistry.addEntry(RepoIdRegistry.emptyRegistry(), {
-        repoId,
-      });
-      RepoIdRegistry.writeRegistry(registry, dirname);
-      return dirname;
-    }
-
     it("prints usage with '--help'", async () => {
-      const exportGraph = makeExportGraph([new MockAnalysisAdapter("foo")]);
+      const exportGraph = makeExportGraph(jest.fn());
       expect(await run(exportGraph, ["--help"])).toEqual({
         exitCode: 0,
         stdout: expect.arrayContaining([
@@ -92,7 +42,7 @@ describe("cli/exportGraph", () => {
     });
 
     it("errors if no repoId is provided", async () => {
-      const exportGraph = makeExportGraph([new MockAnalysisAdapter("foo")]);
+      const exportGraph = makeExportGraph(jest.fn());
       expect(await run(exportGraph, [])).toEqual({
         exitCode: 1,
         stdout: [],
@@ -103,50 +53,29 @@ describe("cli/exportGraph", () => {
       });
     });
 
-    it("throws an error if no plugins are available", async () => {
+    it("attempts to load the repoId provided", async () => {
-      const exportGraph = makeExportGraph([]);
+      const mockFn = jest.fn();
-      expect(await run(exportGraph, ["--help"])).toEqual({
+      const exportGraph = makeExportGraph(mockFn);
-        exitCode: 1,
+      await run(exportGraph, ["foo/bar"]);
-        stdout: [],
+      expect(mockFn).toHaveBeenCalledWith(makeRepoId("foo", "bar"));
-        stderr: [
-          "fatal: no plugins available",
-          "fatal: this is likely a build error",
-        ],
-      });
     });
 
-    it("prints json-serialized graph to stdout for a single plugin", async () => {
+    it("on load success, prints the stringified graph to stdout", async () => {
-      const g = new Graph().addNode(NodeAddress.empty);
+      const graph = new Graph().addNode(NodeAddress.empty);
-      const mockAdapter = new MockAnalysisAdapter("foo", g);
+      const loadGraphResult = {status: "SUCCESS", graph};
-      const exportGraph = makeExportGraph([mockAdapter]);
+      const exportGraph = makeExportGraph(
-      setUpRegistryWithId(makeRepoId("foo", "bar"));
+        (_unused_repoId) => new Promise((resolve) => resolve(loadGraphResult))
+      );
       const result = run(exportGraph, ["foo/bar"]);
       expect(await result).toEqual({
         exitCode: 0,
-        stdout: [stringify(g.toJSON())],
+        stdout: [stringify(graph.toJSON())],
-        stderr: [],
-      });
-    });
-
-    it("merges graphs for multiple plugins", async () => {
-      const g1 = new Graph().addNode(NodeAddress.fromParts(["g1"]));
-      const g2 = new Graph().addNode(NodeAddress.fromParts(["g2"]));
-      const m1 = new MockAnalysisAdapter("foo", g1);
-      const m2 = new MockAnalysisAdapter("bar", g2);
-      const mergedGraph = Graph.merge([g1, g2]);
-      setUpRegistryWithId(makeRepoId("foo", "bar"));
-      const exportGraph = makeExportGraph([m1, m2]);
-      expect(await run(exportGraph, ["foo/bar"])).toEqual({
-        exitCode: 0,
-        stdout: [stringify(mergedGraph.toJSON())],
         stderr: [],
       });
     });
 
     it("errors if multiple repos are provided", async () => {
-      const m1 = new MockAnalysisAdapter("foo");
+      const exportGraph = makeExportGraph(jest.fn());
-      const m2 = new MockAnalysisAdapter("bar");
-      const exportGraph = makeExportGraph([m1, m2]);
       expect(await run(exportGraph, ["foo/bar", "zod/zoink"])).toEqual({
         exitCode: 1,
         stdout: [],
@@ -158,10 +87,10 @@ describe("cli/exportGraph", () => {
     });
 
     it("errors if the repoId was not loaded first", async () => {
-      const g = new Graph().addNode(NodeAddress.empty);
+      const loadGraphResult = {status: "REPO_NOT_LOADED"};
-      const mockAdapter = new MockAnalysisAdapter("mock", g);
+      const exportGraph = makeExportGraph(
-      const exportGraph = makeExportGraph([mockAdapter]);
+        (_unused_repoId) => new Promise((resolve) => resolve(loadGraphResult))
-      setUpRegistryWithId(makeRepoId("foo", "bar"));
+      );
       const result = run(exportGraph, ["zod/zoink"]);
       expect(await result).toEqual({
         exitCode: 1,
@@ -173,30 +102,20 @@ describe("cli/exportGraph", () => {
       });
     });
 
-    it("passes the right arguments to adapter.load", async () => {
-      const mockAdapter = new MockAnalysisAdapter("zoo");
-      const exportGraph = makeExportGraph([mockAdapter]);
-      const repoId = makeRepoId("foo", "bar");
-      // $ExpectFlowError
-      mockAdapter.load = jest.fn();
-      const directory = setUpRegistryWithId(repoId);
-      await run(exportGraph, ["foo/bar"]);
-      expect(mockAdapter.load).toHaveBeenCalledWith(directory, repoId);
-    });
-
     it("reports the failing plugin when a plugin rejects", async () => {
-      const mockAdapter = new MockAnalysisAdapter("bar");
+      const loadGraphResult = {
-      const exportGraph = makeExportGraph([mockAdapter]);
+        status: "PLUGIN_FAILURE",
-      const repoId = makeRepoId("foo", "bar");
+        pluginName: "badPlugin",
-      setUpRegistryWithId(repoId);
+        error: new Error("MockPluginFailure"),
+      };
+      const exportGraph = makeExportGraph(
+        (_unused_repoId) => new Promise((resolve) => resolve(loadGraphResult))
+      );
       const result = await run(exportGraph, ["foo/bar"]);
       expect(result).toEqual({
         exitCode: 1,
         stdout: [],
-        stderr: [
+        stderr: ['fatal: plugin "badPlugin" errored: MockPluginFailure'],
-          'fatal: plugin "bar" errored: MockAnalysisAdapterRejects',
-          "fatal: run 'sourcecred help export-graph' for help",
-        ],
       });
     });
   });