Refactor file ownership (#48)

* adding a jest mock

* silencing logger during tests

* refactoring file manipulation to be a separate concern to ownership

* leaving some todo's for future jjmschofield

Author: jjmschofield

src/lib/ownership/lib/countLinesInFile.ts src/lib/file/countLines.tssrc/lib/ownership/lib/countLinesInFile.ts renamed to src/lib/file/countLines.ts

@@ -1,7 +1,7 @@
 import * as fs from 'fs';
-import { log } from '../../logger';
+import { log } from '../logger';
 
-export const countLinesInFile = async (filePath: string): Promise<number> => {
+export const countLines = async (filePath: string): Promise<number> => {
   let i;
   let count = 0;
   return new Promise((resolve, reject) => {

src/lib/file/getFilePaths.ts

@@ -0,0 +1,26 @@
+import { readGit } from './readGit';
+import { readDir } from './readDir';
+import * as path from 'path';
+
+export enum FILE_DISCOVERY_STRATEGY {
+  FILE_SYSTEM,
+  GIT_LS,
+}
+
+export const getFilePaths = async (dir: string, strategy: FILE_DISCOVERY_STRATEGY, root?: string) => {
+  let filePaths;
+
+  if (strategy === FILE_DISCOVERY_STRATEGY.GIT_LS) {
+    filePaths = await readGit(dir);
+  } else {
+    filePaths = await readDir(dir, ['.git']);
+  }
+
+  if (root) { // We need to re-add the root so that later ops can find the file
+    filePaths = filePaths.map(filePath => path.join(root, filePath));
+  }
+
+  filePaths.sort();
+
+  return filePaths;
+};

src/lib/file/index.ts

@@ -0,0 +1 @@
+export { getFilePaths, FILE_DISCOVERY_STRATEGY } from './getFilePaths';

…ownership/lib/readDirRecursively.test.ts src/lib/file/readDir.test.tssrc/lib/ownership/lib/readDirRecursively.test.ts renamed to src/lib/file/readDir.test.ts src/lib/ownership/lib/readDirRecursively.test.ts renamed to src/lib/file/readDir.test.ts

@@ -1,5 +1,5 @@
 import fs from 'fs';
-import * as underTest from './readDirRecursively';
+import * as underTest from './readDir';
 import * as path from 'path';
 
 jest.mock('fs');
@@ -26,7 +26,7 @@ describe('readDirRecursively', () => {
     statSyncMock.mockReturnValue(statFake(STAT_FAKE_TYPES.FILE));
 
     // Act
-    const result = await underTest.readDirRecursively('root');
+    const result = await underTest.readDir('root');
 
     // Assert
     expect(readdirSyncMock).toHaveBeenCalledWith(path.resolve('root'));
@@ -49,7 +49,7 @@ describe('readDirRecursively', () => {
     statSyncMock.mockReturnValue(statFake(STAT_FAKE_TYPES.FILE));
 
     // Act
-    const result = await underTest.readDirRecursively('root');
+    const result = await underTest.readDir('root');
 
     // Assert
     expect(readdirSyncMock).toHaveBeenCalledWith(path.resolve('root'));
@@ -70,7 +70,7 @@ describe('readDirRecursively', () => {
     statSyncMock.mockReturnValue(statFake(STAT_FAKE_TYPES.FILE));
 
     // Act
-    const result = await underTest.readDirRecursively('root', ['*.js']);
+    const result = await underTest.readDir('root', ['*.js']);
 
     // Assert
     expect(result).toEqual(expectedFiles);
@@ -88,7 +88,7 @@ describe('readDirRecursively', () => {
     statSyncMock.mockReturnValue(statFake(STAT_FAKE_TYPES.FILE));
 
     // Act
-    const result = await underTest.readDirRecursively('root');
+    const result = await underTest.readDir('root');
 
     // Assert
     expect(result).toEqual(expectedFiles);
@@ -113,7 +113,7 @@ describe('readDirRecursively', () => {
 
 
     // Act
-    const result = await underTest.readDirRecursively('root');
+    const result = await underTest.readDir('root');
 
     // Assert
     expect(result).toEqual(expectedFiles);
@@ -138,7 +138,7 @@ describe('readDirRecursively', () => {
 
 
     // Act
-    const result = await underTest.readDirRecursively('root');
+    const result = await underTest.readDir('root');
 
     // Assert
     expect(result).toEqual(expectedFiles);
@@ -164,7 +164,7 @@ describe('readDirRecursively', () => {
 
 
     // Act
-    const result = await underTest.readDirRecursively('root');
+    const result = await underTest.readDir('root');
 
     // Assert
     expect(result).toEqual(expectedFiles);

…/lib/ownership/lib/readDirRecursively.ts src/lib/file/readDir.tssrc/lib/ownership/lib/readDirRecursively.ts renamed to src/lib/file/readDir.ts src/lib/ownership/lib/readDirRecursively.ts renamed to src/lib/file/readDir.ts

@@ -2,7 +2,7 @@ import fs, { Stats } from 'fs';
 import ignore, { Ignore } from 'ignore';
 import path from 'path';
 
-export const readDirRecursively = async (dir: string, filters: string[] = []): Promise<string[]> => {
+export const readDir = async (dir: string, filters: string[] = []): Promise<string[]> => {
   return new Promise((resolve, reject) => {
     try {
       const ignores = ignore().add(filters);

…wnership/lib/readTrackedGitFiles.test.ts src/lib/file/readGit.test.tssrc/lib/ownership/lib/readTrackedGitFiles.test.ts renamed to src/lib/file/readGit.test.ts src/lib/ownership/lib/readTrackedGitFiles.test.ts renamed to src/lib/file/readGit.test.ts

@@ -1,22 +1,22 @@
-import { exec } from '../../util/exec';
-import { readTrackedGitFiles } from './readTrackedGitFiles';
+import { exec } from '../util/exec';
+import { readGit } from './readGit';
 
-jest.mock('../../util/exec');
+jest.mock('../util/exec');
 const execFileMock = exec as jest.Mock;
 
-describe('readTrackedGitFiles', () => {
+describe('readGit', () => {
   it('should return the expected list of files when called', async () => {
     execFileMock.mockResolvedValue({ stdout: 'foo\nbar\n', stderr: '' });
 
-    const result = await readTrackedGitFiles('some/dir');
+    const result = await readGit('some/dir');
 
     expect(result).toStrictEqual(['foo', 'bar']);
   });
 
   it('should call git ls-files with the correct directory', async () => {
     execFileMock.mockResolvedValue({ stdout: '', stderr: '' });
 
-    const result = await readTrackedGitFiles('some/dir');
+    const result = await readGit('some/dir');
 
     expect(exec).toHaveBeenCalledWith(
       'git ls-files',

src/lib/file/readGit.ts

@@ -0,0 +1,6 @@
+import { exec } from '../util/exec';
+
+export const readGit = async (dir: string): Promise<string[]> => {
+  const { stdout } = await exec('git ls-files', { cwd: dir });
+  return stdout.split('\n').filter(x => !!x);
+};

src/lib/ownership/file.test.ts

@@ -2,28 +2,28 @@ import { getFileOwnership } from './file';
 
 import { OwnershipEngine } from './lib/OwnershipEngine';
 
-import { readDirRecursively } from './lib/readDirRecursively';
-import { readTrackedGitFiles } from './lib/readTrackedGitFiles';
-import { countLinesInFile } from './lib/countLinesInFile';
+import { readDir } from '../file/readDir';
+import { readGit } from '../file/readGit';
+import { countLines } from '../file/countLines';
 import { OwnedFile } from './lib/OwnedFile';
 
 jest.mock('./lib/OwnershipEngine');
-jest.mock('./lib/readDirRecursively');
-jest.mock('./lib/readTrackedGitFiles');
-jest.mock('./lib/countLinesInFile');
+jest.mock('../file/readDir');
+jest.mock('../file/readGit');
+jest.mock('../file/countLines');
 
 describe('file', () => {
   const mocks = {
-    readDirRecursively: readDirRecursively as jest.Mock,
-    readTrackedGitFiles: readTrackedGitFiles as jest.Mock,
-    countLinesInFile: countLinesInFile as jest.Mock,
+    readDir: readDir as jest.Mock,
+    readGit: readGit as jest.Mock,
+    countLines: countLines as jest.Mock,
   };
 
   beforeEach(() => {
     jest.resetAllMocks();
-    mocks.readDirRecursively.mockResolvedValue([]);
-    mocks.readTrackedGitFiles.mockResolvedValue([]);
-    mocks.countLinesInFile.mockResolvedValue(0);
+    mocks.readDir.mockResolvedValue([]);
+    mocks.readGit.mockResolvedValue([]);
+    mocks.countLines.mockResolvedValue(0);
   });
 
   describe('getFileOwnership', () => {
@@ -46,18 +46,18 @@ describe('file', () => {
       await getFileOwnership({ codeowners: expected, dir: expected, onlyGit: false });
 
       // Assert
-      expect(mocks.readDirRecursively).toHaveBeenCalledWith(expected, ['.git']);
+      expect(mocks.readDir).toHaveBeenCalledWith(expected, ['.git']);
     });
 
-    it('should read files from git tracked files when onlyGit is specified', async () => {
+    it('should read files from git tracked filegs when onlyGit is specified', async () => {
       // Arrange
       const expected = 'some/dir';
 
       // Act
       await getFileOwnership({ codeowners: expected, dir: expected, onlyGit: true });
 
       // Assert
-      expect(mocks.readTrackedGitFiles).toHaveBeenCalledWith(expected);
+      expect(mocks.readGit).toHaveBeenCalledWith(expected);
     });
 
     it('should return owned files', async () => {
@@ -79,7 +79,7 @@ describe('file', () => {
         };
       });
 
-      mocks.readDirRecursively.mockResolvedValue(expected.map(f => f.path).reverse());
+      mocks.readDir.mockResolvedValue(expected.map(f => f.path).reverse());
 
       // Act
       const result = await getFileOwnership({ codeowners: 'some/file', dir: '/', onlyGit: false });

src/lib/ownership/file.ts

@@ -1,41 +1,25 @@
-import * as path from 'path';
 import { OwnedFile } from './lib/OwnedFile';
 import { OwnershipEngine } from './lib/OwnershipEngine';
-import { readDirRecursively } from './lib/readDirRecursively';
-import { readTrackedGitFiles } from './lib/readTrackedGitFiles';
-import { countLinesInFile } from './lib/countLinesInFile';
+import { countLines } from '../file/countLines';
+import { getFilePaths, FILE_DISCOVERY_STRATEGY } from '../file';
 
 export const getFileOwnership = async (options: { codeowners: string, dir: string, onlyGit: boolean, root?: string }): Promise<OwnedFile[]> => {
-  const filePaths = await getFilePaths(options.dir, options.onlyGit, options.root);
+  // TODO - make getting file paths a concern for the caller
+  const strategy = options.onlyGit ? FILE_DISCOVERY_STRATEGY.GIT_LS : FILE_DISCOVERY_STRATEGY.FILE_SYSTEM;
+  const filePaths = await getFilePaths(options.dir, strategy, options.root);
 
   const engine = OwnershipEngine.FromCodeownersFile(options.codeowners);
 
   const files: OwnedFile[] = [];
 
   for (const filePath of filePaths) {
     const owners = engine.calcFileOwnership(filePath);
-    const lines = await countLinesInFile(filePath);
+
+    // TODO - make counting lines a concern for the caller or for stats
+    const lines = await countLines(filePath);
 
     files.push(new OwnedFile({ path: filePath, owners, lines }));
   }
 
   return files;
 };
-
-const getFilePaths = async (dir: string, onlyGit: boolean, root?: string) => {
-  let filePaths;
-
-  if (onlyGit) {
-    filePaths = await readTrackedGitFiles(dir);
-  } else {
-    filePaths = await readDirRecursively(dir, ['.git']);
-  }
-
-  if (root) { // We need to re-add the root so that later ops can find the file
-    filePaths = filePaths.map(filePath => path.join(root, filePath));
-  }
-
-  filePaths.sort();
-
-  return filePaths;
-};

src/lib/ownership/lib/OwnedFile.ts

@@ -1,5 +1,5 @@
 import { OwnershipEngine } from './OwnershipEngine';
-import { countLinesInFile } from './countLinesInFile';
+import { countLines } from '../../file/countLines';
 
 export class OwnedFile {
   // tslint:disable-next-line:variable-name
@@ -33,11 +33,12 @@ export class OwnedFile {
     return `${line}\n`;
   }
 
+  // TODO - remove this, keep this class pure
   // tslint:disable-next-line:variable-name
   public static FromPath = async (filePath: string, engine: OwnershipEngine, opts = { countLines: true }) => {
     return new OwnedFile({
       path: filePath,
-      lines: opts.countLines ? await countLinesInFile(filePath) : 0,
+      lines: opts.countLines ? await countLines(filePath) : 0,
       owners: engine.calcFileOwnership(filePath),
     });
   }