Fixded a bug in circular dep detection logic.

Also moved util.js to util/index.js, along with some misc cleanup.

src/garden.ts

@@ -95,7 +95,7 @@ import { Task } from "./types/task"
 import {
   LocalConfigStore,
 } from "./config-store"
-import { detectCircularDependencies } from "./algo"
+import { detectCircularDependencies } from "./util/detectCycles"
 
 export interface ModuleMap<T extends Module> {
   [key: string]: T

src/plugins/kubernetes/kubectl.ts

@@ -47,7 +47,7 @@ export class Kubectl {
     args: string[],
     { data, ignoreError = false, silent = true, timeout = KUBECTL_DEFAULT_TIMEOUT }: KubectlParams = {},
   ): Promise<KubectlOutput> {
-    // TODO: use the spawn helper from util.ts
+    // TODO: use the spawn helper from index.ts
     const logger = getLogger()
     const out: KubectlOutput = {
       code: 0,

src/algo.ts → src/util/detectCycles.ts

@@ -7,10 +7,10 @@
  */
 
 import { forEach, get, isEqual, join, set, uniqWith } from "lodash"
-import { Module, ModuleConfig } from "./types/module"
-import { ConfigurationError } from "./exceptions"
+import { Module, ModuleConfig } from "../types/module"
+import { ConfigurationError } from "../exceptions"
 
-type Cycle = string[]
+export type Cycle = string[]
 
 /*
   Implements a variation on the Floyd-Warshall algorithm to compute minimal cycles.
@@ -65,7 +65,7 @@ export async function detectCircularDependencies(modules: Module[], serviceNames
   }
 }
 
-function detectCycles(graph, vertices: string[]): Cycle[] {
+export function detectCycles(graph, vertices: string[]): Cycle[] {
   // Compute shortest paths
   for (const k of vertices) {
     for (const i of vertices) {
@@ -88,10 +88,12 @@ function detectCycles(graph, vertices: string[]): Cycle[] {
       cycle.push(nextInCycle)
       nextInCycle = next(graph, nextInCycle, v)
     }
-    return cycle.sort()
+    return cycle
   })
 
-  return uniqWith(cycles, isEqual)
+  return uniqWith(
+    cycles, // The concat calls below are to prevent in-place sorting.
+    (c1, c2) => isEqual(c1.concat().sort(), c2.concat().sort()))
 }
 
 function distance(graph, source, destination): number {

src/util.ts → src/util/index.ts

@@ -18,16 +18,16 @@ import * as inquirer from "inquirer"
 import { spawn as _spawn } from "child_process"
 import { existsSync, readFileSync, writeFileSync } from "fs"
 import { join } from "path"
-import { getLogger, RootLogNode } from "./logger"
+import { getLogger, RootLogNode } from "../logger/index"
 import {
   TimeoutError,
   GardenError,
-} from "./exceptions"
+} from "../exceptions"
 import { PassThrough } from "stream"
 import { isArray, isPlainObject, extend, mapValues, pickBy } from "lodash"
 import highlight from "cli-highlight"
 import chalk from "chalk"
-import { FancyConsoleWriter } from "./logger/writers"
+import { FancyConsoleWriter } from "../logger/writers"
 import hasAnsi = require("has-ansi")
 
 // shim to allow async generator functions

test/helpers.ts

@@ -201,15 +201,6 @@ export async function expectError(fn: Function, typeOrCallback: string | ((err:
   }
 }
 
-export async function expectNoError(fn: Function) {
-  try {
-    await fn()
-  } catch (err) {
-    throw new Error(`Did not expect an error, but got ${err}`)
-  }
-  return
-}
-
 export function taskResultOutputs(results: TaskResults) {
   return mapValues(results, r => r.output)
 }

test/src/util.ts

@@ -1,6 +1,8 @@
-import { scanDirectory } from "../../src/util"
-import { join } from "path"
 import { expect } from "chai"
+import { join } from "path"
+import { expectError, makeTestGarden } from "../helpers"
+import { scanDirectory } from "../../src/util"
+import { detectCycles } from "../../src/util/detectCycles"
 
 describe("util", () => {
   describe("scanDirectory", () => {
@@ -32,5 +34,41 @@ describe("util", () => {
 
       expect(count).to.eq(3)
     })
+
+    it("should throw an exception when circular dependencies are present", async () => {
+      const circularProjectRoot = join(__dirname, "..", "data", "test-project-circular-deps")
+      const garden = await makeTestGarden(circularProjectRoot)
+      await expectError(
+        async () => await garden.scanModules(),
+        "configuration")
+    })
+
+    it("should not throw an exception when no circular dependencies are present", async () => {
+      const nonCircularProjectRoot = join(__dirname, "..", "data", "test-project-b")
+      const garden = await makeTestGarden(nonCircularProjectRoot)
+      expect(async () => { await garden.scanModules() }).to.not.throw()
+    })
+  })
+
+  describe("detectCycles", () => {
+
+    it("should detect self-to-self cycles", () => {
+      const cycles = detectCycles({
+        a: {a: {distance: 1, next: "a"}},
+      }, ["a"])
+
+      expect(cycles).to.deep.eq([["a"]])
+    })
+
+    it("should preserve dependency order when returning cycles", () => {
+      const cycles = detectCycles({
+        foo: {bar: {distance: 1, next: "bar"}},
+        bar: {baz: {distance: 1, next: "baz"}},
+        baz: {foo: {distance: 1, next: "foo"}},
+      }, ["foo", "bar", "baz"])
+
+      expect(cycles).to.deep.eq([["foo", "bar", "baz"]])
+    })
+
   })
 })