Merge pull request #4 from tlaanemaa/err_refactor

Refactored how error handling works
tlaanemaa · May 4, 2019 · 24933d3 · 24933d3
2 parents 3e6cd6c + 86a6458
commit 24933d3
Show file tree

Hide file tree

Showing 10 changed files with 99 additions and 164 deletions.
diff --git a/bin/run.js b/bin/run.js
@@ -1,2 +1,19 @@
 #!/usr/bin/env node
-require('..')();
+
+const logAndExit = (err) => {
+  // eslint-disable-next-line no-console
+  console.error(err instanceof Error ? err.message : err);
+  process.exit(1);
+};
+
+// Global error handlers
+process.on('uncaughtException', logAndExit);
+process.on('unhandledRejection', logAndExit);
+
+(async () => {
+  // eslint-disable-next-line global-require
+  await require('..')();
+
+  // Safeguard against hanging application
+  process.exit();
+})();
diff --git a/src/commands.js b/src/commands.js
@@ -0,0 +1,34 @@
+const { containers: containerNames } = require('./modules/options');
+const { getContainers, restoreContainer, backupContainer } = require('./modules/docker');
+const { getInspectFilesSync, logAndReturnErrors } = require('./modules/utils');
+
+// Main backup function
+const backup = async () => {
+  // Get all container names if needed
+  const containers = containerNames.length
+    ? containerNames
+    : await getContainers();
+
+  // Backup containers
+  return Promise.all(containers.map(
+    logAndReturnErrors(backupContainer),
+  ));
+};
+
+// Main restore function
+const restore = async () => {
+  // Get all container names if needed
+  const containers = containerNames.length
+    ? containerNames
+    : getInspectFilesSync();
+
+  // Restore containers
+  return Promise.all(containers.map(
+    logAndReturnErrors(restoreContainer),
+  ));
+};
+
+module.exports = {
+  backup,
+  restore,
+};
diff --git a/src/main.js b/src/main.js
@@ -1,46 +1,19 @@
-const { operation, containers: containerNames } = require('./modules/options');
-const { getContainers, restoreContainer, backupContainer } = require('./modules/docker');
-const { getInspectFilesSync, asyncTryLog } = require('./modules/utils');
-
-// Main backup function
-const backup = async () => {
-  const containers = containerNames.length
-    ? containerNames
-    : await asyncTryLog(() => getContainers(), true);
-
-  return Promise.all(containers.map(
-    container => asyncTryLog(() => backupContainer(container)),
-  ));
-};
-
-// Main restore function
-const restore = async () => {
-  const containers = containerNames.length
-    ? containerNames
-    : getInspectFilesSync();
-
-  return Promise.all(containers.map(
-    container => asyncTryLog(() => restoreContainer(container)),
-  ));
-};
+const commands = require('./commands');
+const { operation } = require('./modules/options');
 
 // Main method to run the tool
 module.exports = async () => {
-  const operations = { backup, restore };
-  const results = await operations[operation]();
+  const results = await commands[operation]();
   // eslint-disable-next-line no-console
   console.log('== Done ==');
 
-  // Check if we had any errors and log them again if there are
+  // Check if we had any errors and throw them if we did
   const errors = results.filter(result => result instanceof Error);
   if (errors.length) {
-    // eslint-disable-next-line no-console
-    console.error('\nThe following errors occurred during the run (this does not include errors from the tar command used for volume backup/restore):');
-    // eslint-disable-next-line no-console
-    errors.map(err => console.error(err.message));
-    process.exit(1);
+    const errorHeader = '\nThe following errors occurred during the run (this does not include errors from the tar command used for volume backup/restore):\n';
+    const errorMessages = errors.map(e => e.message).join('\n');
+    throw new Error(errorHeader + errorMessages);
   }
 
-  process.exit(0);
   return results;
 };
diff --git a/src/modules/folderStructure.js b/src/modules/folderStructure.js
@@ -4,14 +4,8 @@ const { directory } = require('./options');
 
 // Ensure folder existence
 const ensureFolderExistsSync = (dir) => {
-  try {
-    if (!fs.existsSync(dir)) {
-      fs.mkdirSync(dir);
-    }
-  } catch (e) {
-    // eslint-disable-next-line no-console
-    console.error(e.message);
-    process.exit(1);
+  if (!fs.existsSync(dir)) {
+    fs.mkdirSync(dir);
   }
 };
 

diff --git a/src/modules/options.js b/src/modules/options.js
@@ -35,18 +35,15 @@ program
 program
   .command('*', { noHelp: true })
   .action((command) => {
-    // eslint-disable-next-line no-console
-    console.error(`Unknown operation: ${command}`, '\nUse --help to see all options');
-    process.exit(1);
+    throw new Error(`Unknown operation: ${command}\nUse --help to see all options`);
   });
 
 // Parse args
 program.parse(process.argv);
 
 // Show help if no operation is provided
 if (!commandArgs.operation) {
-  program.outputHelp();
-  process.exit(1);
+  program.help();
 }
 
 module.exports = { ...commandArgs, ...program.opts() };
diff --git a/src/modules/utils.js b/src/modules/utils.js
@@ -12,15 +12,8 @@ const formatContainerName = name => name.replace(/^\//g, '');
 
 // Get contents of a folder synchronously
 const getFilesSync = (folder, extension) => {
-  try {
-    const files = fs.readdirSync(folder);
-    return files.filter(file => path.extname(file) === extension);
-  } catch (e) {
-    // eslint-disable-next-line no-console
-    console.error(e.message);
-    process.exit(1);
-    throw e;
-  }
+  const files = fs.readdirSync(folder);
+  return files.filter(file => path.extname(file) === extension);
 };
 
 // Get all container inspect backups synchronously
@@ -46,16 +39,13 @@ const saveInspect = (inspect) => {
 const getVolumeFilesSync = () => getFilesSync(folderStructure.volumes, '.tar')
   .map(file => path.basename(file, path.extname(file)));
 
-// Helper to catch and log errors on async functions
-const asyncTryLog = async (func, exit = false) => {
+// Helper to catch errors, log and then return them
+const logAndReturnErrors = func => async (...args) => {
   try {
-    return await func();
+    return await func(...args);
   } catch (e) {
     // eslint-disable-next-line no-console
     console.error(e.message);
-    if (exit) {
-      process.exit(1);
-    }
     return e;
   }
 };
@@ -67,5 +57,5 @@ module.exports = {
   loadInspect,
   saveInspect,
   getVolumeFilesSync,
-  asyncTryLog,
+  logAndReturnErrors,
 };
diff --git a/test/main.test.js b/test/main.test.js
@@ -11,45 +11,45 @@ describe('backup', () => {
     const options = require('../src/modules/options');
     options.containers = [];
     const docker = require('../src/modules/docker');
-    global.process.exit = jest.fn();
     const main = require('../src/main');
 
     const result = await main();
 
     expect(result).toEqual([true, true, true]);
     expect(docker.getContainers).toHaveBeenCalledTimes(1);
     expect(docker.backupContainer).toHaveBeenCalledTimes(3);
-    expect(docker.backupContainer).toHaveBeenLastCalledWith(8);
-    expect(global.process.exit).toHaveBeenCalledWith(0);
+    expect(docker.backupContainer)
+      .toHaveBeenLastCalledWith(8, expect.any(Number), expect.any(Array));
   });
 
   it('should backup the given container', async () => {
     const options = require('../src/modules/options');
     options.containers = ['pear'];
     const docker = require('../src/modules/docker');
-    global.process.exit = jest.fn();
     const main = require('../src/main');
 
     await main();
 
     expect(docker.getContainers).toHaveBeenCalledTimes(0);
     expect(docker.backupContainer).toHaveBeenCalledTimes(1);
-    expect(docker.backupContainer).toHaveBeenLastCalledWith('pear');
-    expect(global.process.exit).toHaveBeenCalledWith(0);
+    expect(docker.backupContainer)
+      .toHaveBeenLastCalledWith('pear', expect.any(Number), expect.any(Array));
   });
 
   it('should catch errors and return false', async () => {
+    expect.assertions(1);
     const options = require('../src/modules/options');
     options.containers = ['pear'];
     const docker = require('../src/modules/docker');
-    const mockError = new Error('Mock backup error');
-    docker.backupContainer = () => { throw mockError; };
-    global.process.exit = jest.fn();
+    docker.backupContainer = () => { throw new Error('Mock backup error'); };
     const main = require('../src/main');
 
-    const result = await main();
-    expect(result).toEqual([mockError]);
-    expect(global.process.exit).toHaveBeenCalledWith(0);
+    try {
+      await main();
+    } catch (e) {
+      expect(e.message)
+        .toBe('\nThe following errors occurred during the run (this does not include errors from the tar command used for volume backup/restore):\nMock backup error');
+    }
   });
 });
 
@@ -60,7 +60,6 @@ describe('restore', () => {
     options.operation = 'restore';
     options.containers = [];
     const docker = require('../src/modules/docker');
-    global.process.exit = jest.fn();
     const main = require('../src/main');
 
     const result = await main();
@@ -69,8 +68,8 @@ describe('restore', () => {
     expect(fs.readdirSync).toHaveBeenCalledTimes(1);
     expect(fs.readdirSync).toHaveBeenCalledWith('/folder/containers');
     expect(docker.restoreContainer).toHaveBeenCalledTimes(3);
-    expect(docker.restoreContainer).toHaveBeenLastCalledWith('c');
-    expect(global.process.exit).toHaveBeenCalledWith(0);
+    expect(docker.restoreContainer)
+      .toHaveBeenLastCalledWith('c', expect.any(Number), expect.any(Array));
   });
 
   it('should restore the given container', async () => {
@@ -79,29 +78,30 @@ describe('restore', () => {
     options.operation = 'restore';
     options.containers = ['mango'];
     const docker = require('../src/modules/docker');
-    global.process.exit = jest.fn();
     const main = require('../src/main');
 
     await main();
 
     expect(fs.readdir).toHaveBeenCalledTimes(0);
     expect(docker.restoreContainer).toHaveBeenCalledTimes(1);
-    expect(docker.restoreContainer).toHaveBeenLastCalledWith('mango');
-    expect(global.process.exit).toHaveBeenCalledWith(0);
+    expect(docker.restoreContainer)
+      .toHaveBeenLastCalledWith('mango', expect.any(Number), expect.any(Array));
   });
 
   it('should catch errors and return false', async () => {
+    expect.assertions(1);
     const options = require('../src/modules/options');
     options.operation = 'restore';
     options.containers = ['pear'];
     const docker = require('../src/modules/docker');
-    const mockError = new Error('Mock restore error');
-    docker.restoreContainer = () => { throw mockError; };
-    global.process.exit = jest.fn();
+    docker.restoreContainer = () => { throw new Error('Mock restore error'); };
     const main = require('../src/main');
 
-    const result = await main();
-    expect(result).toEqual([mockError]);
-    expect(global.process.exit).toHaveBeenCalledWith(0);
+    try {
+      await main();
+    } catch (e) {
+      expect(e.message)
+        .toBe('\nThe following errors occurred during the run (this does not include errors from the tar command used for volume backup/restore):\nMock restore error');
+    }
   });
 });
diff --git a/test/modules/folderStructure.test.js b/test/modules/folderStructure.test.js
@@ -13,17 +13,4 @@ describe('folderStructure', () => {
 
     expect(fs.mkdirSync).toHaveBeenCalledTimes(0);
   });
-
-  it('should log and exit on error', () => {
-    const mockError = new Error('Mock error');
-    const fs = require('fs');
-    fs.mkdirSync = () => { throw mockError; };
-    global.console.error = jest.fn();
-    global.process.exit = jest.fn();
-
-    require('../../src/modules/folderStructure');
-
-    expect(global.console.error).toHaveBeenCalledWith('Mock error');
-    expect(global.process.exit).toHaveBeenCalledWith(1);
-  });
 });
diff --git a/test/modules/options.test.js b/test/modules/options.test.js
@@ -20,42 +20,20 @@ describe('options', () => {
     expect(options.containers).toEqual(['mango']);
   });
 
-  it('should show error and exit on unknown operation', () => {
-    /*
-      This test runs over both the unknown an omitted operation scenarios because
-      both use process.exit(1) but since we mock it, it will run over it and not exit.
-      Thus triggering both paths, which in real usage is not possible
-    */
+  it('should throw error on unknown operation', () => {
     global.process.argv = ['node', 'dummy.js', 'dance', 'mango'];
-    global.console.error = jest.fn();
-    global.process.exit = jest.fn();
-    const { Command } = require('commander');
-    Command.prototype.outputHelp = jest.fn();
-
-    require('../../src/modules/options');
 
-    expect(global.console.error).toHaveBeenCalledTimes(1);
-    expect(global.console.error).toHaveBeenCalledWith(
-      'Unknown operation: dance',
-      '\nUse --help to see all options',
-    );
-    expect(global.process.exit).toHaveBeenCalledTimes(2);
-    expect(global.process.exit).toHaveBeenCalledWith(1);
-    expect(Command.prototype.outputHelp).toHaveBeenCalledTimes(1);
+    expect(() => require('../../src/modules/options'))
+      .toThrow('Unknown operation: dance\nUse --help to see all options');
   });
 
-  it('should show help and exit if operation is omitted', () => {
+  it('should show help if operation is omitted', () => {
     global.process.argv = ['node', 'dummy.js'];
-    global.console.error = jest.fn();
-    global.process.exit = jest.fn();
     const { Command } = require('commander');
-    Command.prototype.outputHelp = jest.fn();
+    Command.prototype.help = jest.fn();
 
     require('../../src/modules/options');
 
-    expect(global.console.error).toHaveBeenCalledTimes(0);
-    expect(global.process.exit).toHaveBeenCalledTimes(1);
-    expect(global.process.exit).toHaveBeenCalledWith(1);
-    expect(Command.prototype.outputHelp).toHaveBeenCalledTimes(1);
+    expect(Command.prototype.help).toHaveBeenCalledTimes(1);
   });
 });