From 7c4ff66907496ebe6ec9f4eadd8ddedb1e1ba041 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dandelion=20Man=C3=A9?= Date: Sat, 1 Jun 2019 01:40:21 +0300 Subject: [PATCH] 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 | 132 ++++++++++++++++++++++++++----------------- src/cli/load.test.js | 19 ++++++- 2 files changed, 97 insertions(+), 54 deletions(-) diff --git a/src/cli/load.js b/src/cli/load.js index 6d114ec..85fb530 100644 --- a/src/cli/load.js +++ b/src/cli/load.js @@ -172,60 +172,77 @@ export function makeLoadCommand( }; } -export const loadDefaultPlugins = async (options: LoadOptions) => { - const sourcecredCommand = (args) => [ - process.execPath, - "--max_old_space_size=8192", - 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: [], - })), - ]; +export type SaveTimestamps = ( + $ReadOnlyArray, + RepoId +) => Promise; - const {success: loadSuccess} = await execDependencyGraph(tasks, { - taskPassLabel: "DONE", - }); - if (!loadSuccess) { - throw new Error("Load tasks failed."); - } - addToRepoIdRegistry(options.output); - 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: [], +/** + * A wrapper around the default plugin loader. + * + * We wrap so that we can dependency inject saveTimestamps for testability. + * (saveTimestamps depends on the directory state that's generated by running + * load.) + */ +export const makeLoadDefaultPlugins = (saveTimestamps: SaveTimestamps) => { + return async (options: LoadOptions) => { + const sourcecredCommand = (args) => [ + process.execPath, + "--max_old_space_size=8192", + 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", + }); + 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, 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; diff --git a/src/cli/load.test.js b/src/cli/load.test.js index e573a9b..d56193f 100644 --- a/src/cli/load.test.js +++ b/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({