Merge pull request #232 from ngarbezza/230-bug

bug: before() and after() failing with poor feedback

lib/test.js

@@ -17,11 +17,14 @@ class Test {
     await TestResult.evaluate(this, context);
   }
 
-  async evaluate() {
+  async evaluate(context) {
+    await this._evaluateHook(context.hooks.before);
     try {
       await this.body().call();
     } catch (error) {
       this.setResult(TestResult.error(error));
+    } finally {
+      await this._evaluateHook(context.hooks.after);
     }
   }
 
@@ -128,6 +131,14 @@ class Test {
       throw new Error('Test does not have a valid body');
     }
   }
+
+  async _evaluateHook(hook) {
+    try {
+      hook && await hook.call();
+    } catch (error) {
+      this.setResult(TestResult.error(error));
+    }
+  }
 }
 
 module.exports = Test;

lib/test_result.js

@@ -88,7 +88,7 @@ class TestWithDefinition extends TestResult {
   }
 
   static async handle(test, context) {
-    await test.evaluate();
+    await test.evaluate(context);
     const possibleResults = [TestWithoutAssertion, TestErrored, TestExplicitlyMarkedPending, TestSucceeded, TestFailed];
     // All results evaluate synchronously at this point, so we don't need to async/await this part of the code.
     possibleResults

lib/test_suite.js

@@ -154,20 +154,21 @@ class TestSuite {
     this._body.call();
   }
 
-  async _evaluateHook(hook) {
-    hook && await hook.call();
-  }
-
   async _runTests(context) {
     if (context.randomOrderMode) {
       this._randomizeTests();
     }
+
+    context.hooks = {
+      [TestSuite.BEFORE_HOOK_NAME]: this._before,
+      [TestSuite.AFTER_HOOK_NAME]: this._after,
+    };
+
     // eslint-disable-next-line no-restricted-syntax
     for (const test of this.tests()) {
       this._currentTest = test;
-      await this._evaluateHook(this._before);
+
       await test.run(context);
-      await this._evaluateHook(this._after);
     }
   }
 }

lib/utils.js

@@ -113,6 +113,8 @@ const subclassResponsibility = () => {
   throw new Error('subclass responsibility');
 };
 
+const errorDetailOf = thrownObject => (thrownObject instanceof Error) ? thrownObject.stack : thrownObject.toString();
+
 module.exports = {
   // comparison on objects
   isCyclic,
@@ -136,4 +138,6 @@ module.exports = {
   allFilesMatching,
   // object orientation
   subclassResponsibility,
+  // errors
+  errorDetailOf,
 };

tests/core/test_test.js

@@ -2,13 +2,17 @@
 
 const { suite, test, assert } = require('../../testy');
 const { aTestWithNoAssertions, aTestWithBody } = require('../support/tests_factory');
-const { withRunner, runSingleTest } = require('../support/runner_helpers');
+const { withRunner, runSingleTest, resultOfASuiteWith } = require('../support/runner_helpers');
 const { expectErrorOn, expectFailureOn } = require('../support/assertion_helpers');
 
 const Test = require('../../lib/test');
 const { I18nMessage } = require('../../lib/i18n');
 
 suite('tests behavior', () => {
+  const noop = () => {
+    // intentionally empty function
+  };
+
   test('running a test that does not have any assertion generates an error with a descriptive message', async() => {
     await withRunner(async runner => {
       const testToRun = aTestWithNoAssertions();
@@ -54,4 +58,30 @@ suite('tests behavior', () => {
       expectFailureOn(result, I18nMessage.of('expectation_be_not_empty', '[]'));
     });
   });
+
+  test('a before() hook that fails makes the test fail', async() => {
+    await withRunner(async(runner, assert) => {
+      const test = aTestWithBody(() => assert.isEmpty([]));
+      const before = () => {
+        throw 'oops I did it again';
+      };
+
+      const result = await resultOfASuiteWith(runner, test, before, noop);
+
+      expectErrorOn(result, 'oops I did it again');
+    });
+  });
+
+  test('an after() hook that fails makes the test fail', async() => {
+    await withRunner(async(runner, assert) => {
+      const test = aTestWithBody(() => assert.isEmpty([]));
+      const after = () => {
+        throw 'oops I did it again';
+      };
+
+      const result = await resultOfASuiteWith(runner, test, noop, after);
+
+      expectErrorOn(result, 'oops I did it again');
+    });
+  });
 });

tests/support/runner_helpers.js

@@ -34,8 +34,20 @@ const resultOfATestWith = async assertBlock =>
     return testToRun.result();
   });
 
+const resultOfASuiteWith = async(runner, test, before, after) => {
+  const emptySuiteCallbacks = { onStart: noop, onFinish: noop };
+  const suite = new TestSuite(`suite for ${test.name()}`, noop, emptySuiteCallbacks);
+  suite.addTest(test);
+  suite.before(before);
+  suite.after(after);
+  runner.addSuite(suite);
+  await runner.run();
+  return test.result();
+};
+
 module.exports = {
   withRunner,
   runSingleTest,
   resultOfATestWith,
+  resultOfASuiteWith,
 };

tests/utils_test.js

@@ -9,7 +9,7 @@ suite('utility functions', () => {
     obj.yourself = obj;
     assert.isTrue(Utils.isCyclic(obj));
   });
-  
+
   test('isCyclic is false if the object does not have a cyclic reference', () => {
     const obj = { asd: 2 };
     assert.isFalse(Utils.isCyclic(obj));
@@ -24,47 +24,47 @@ suite('utility functions', () => {
   test('isCyclic does not fail if the object contains properties with null values', () => {
     assert.isFalse(Utils.isCyclic({ thisValueIs: null }));
   });
-  
+
   test('number of elements of Set', () => {
     assert.areEqual(Utils.numberOfElements(new Set([1, 2])), 2);
   });
-  
+
   test('number of elements of Array', () => {
     assert.areEqual(Utils.numberOfElements([1, 2, 3]), 3);
   });
-  
+
   test('isRegex returns true for regexes', () => {
     assert.isTrue(Utils.isRegex(/something/));
     assert.isTrue(Utils.isRegex(new RegExp('something')));
   });
-  
+
   test('isRegex returns false for other objects', () => {
     assert.isFalse(Utils.isRegex({}));
     assert.isFalse(Utils.isRegex('hey'));
     assert.isFalse(Utils.isRegex(() => {}));
   });
-  
+
   test('respondsTo() is false when the property does not exist in the object', () => {
     assert.isFalse(Utils.respondsTo({}, 'dance'));
   });
-  
+
   test('respondsTo() is true when the property exists as a function in the object', () => {
     const thingThatKnowsHowToDance = { dance() {
       return 'I am dancing!';
     } };
     assert.isTrue(Utils.respondsTo(thingThatKnowsHowToDance, 'dance'));
   });
-  
+
   test('respondsTo() is false when the property exists in the object but it is not a function', () => {
     const thingThatHasADanceProperty = { dance: true };
     assert.isFalse(Utils.respondsTo(thingThatHasADanceProperty, 'dance'));
   });
-  
+
   test('respondsTo() is false object is null or undefined', () => {
     assert.isFalse(Utils.respondsTo(null, 'dance'));
     assert.isFalse(Utils.respondsTo(undefined, 'dance'));
   });
-  
+
   test('respondsTo() is true when the property exists as function at class-level, not instance-level', () => {
     class Dancer {
       dance() {
@@ -74,20 +74,20 @@ suite('utility functions', () => {
     const aDancer = new Dancer();
     assert.isTrue(Utils.respondsTo(aDancer, 'dance'));
   });
-  
+
   test('respondsTo() works for non-object types', () => {
     assert.isTrue(Utils.respondsTo('hey', 'toString'));
   });
-  
+
   test('prettyPrint() uses toString() when available', () => {
     const printable = { toString: () => 'I know how to print myself' };
     assert.that(Utils.prettyPrint(printable)).isEqualTo('I know how to print myself');
   });
-  
+
   test('prettyPrint() does not use toString() of arrays, and uses inspect instead', () => {
     assert.that(Utils.prettyPrint([1, 2, 3])).isEqualTo('[ 1, 2, 3 ]');
   });
-  
+
   test('prettyPrint() does not use toString() of strings, and uses inspect instead', () => {
     assert.that(Utils.prettyPrint('hello')).isEqualTo('\'hello\'');
   });
@@ -96,37 +96,52 @@ suite('utility functions', () => {
     const obj = { p1: { p2: { p3: { p4: { p5: true } } } } };
     assert.that(Utils.prettyPrint(obj)).isEqualTo('{ p1: { p2: { p3: { p4: { p5: true } } } } }');
   });
-  
+
   test('notNullOrUndefined() is false on null and undefined', () => {
     assert.isFalse(Utils.notNullOrUndefined(null));
     assert.isFalse(Utils.notNullOrUndefined(undefined));
   });
-  
+
   test('notNullOrUndefined() is true on objects and primitives', () => {
     assert.isTrue(Utils.notNullOrUndefined(5));
     assert.isTrue(Utils.notNullOrUndefined({ an: 'object' }));
   });
-  
+
   test('isUndefined() is true on undefined', () => {
     assert.isTrue(Utils.isUndefined(undefined));
   });
-  
+
   test('isUndefined() is false on null, objects and primitives', () => {
     assert.isFalse(Utils.isUndefined(null));
     assert.isFalse(Utils.isUndefined(0));
     assert.isFalse(Utils.isUndefined(''));
     assert.isFalse(Utils.isUndefined([]));
   });
-  
+
   test('isStringWithContent() is true for a string containing one or more characters', () => {
     assert.isTrue(Utils.isStringWithContent('a'));
     assert.isTrue(Utils.isStringWithContent('an'));
     assert.isTrue(Utils.isStringWithContent('an object'));
   });
-  
+
   test('isStringWithContent() is false for an empty string or string containing only separators', () => {
     assert.isFalse(Utils.isStringWithContent(''));
     assert.isFalse(Utils.isStringWithContent(' '));
     assert.isFalse(Utils.isStringWithContent('   '));
   });
+
+  test('errorDetailOf() returns stack trace if the thrown object is an error', () => {
+    const thrownObject = new Error('oops I did it again');
+    assert.areEqual(Utils.errorDetailOf(thrownObject), thrownObject.stack);
+  });
+
+  test('errorDetailOf() returns the string contents if the thrown object is a string', () => {
+    const thrownObject = 'oops I did it again';
+    assert.areEqual(Utils.errorDetailOf(thrownObject), 'oops I did it again');
+  });
+
+  test('errorDetailOf() returns the string representation if the thrown object is not a string', () => {
+    const thrownObject = [1, 2, 3];
+    assert.areEqual(Utils.errorDetailOf(thrownObject), '1,2,3');
+  });
 });

testy.js

@@ -4,7 +4,7 @@ const libDir = './lib';
 const TestRunner = require(`${libDir}/test_runner`);
 const { Asserter, FailureGenerator, PendingMarker } = require(`${libDir}/asserter`);
 const ConsoleUI = require(`${libDir}/console_ui`);
-const { allFilesMatching, resolvePathFor } = require(`${libDir}/utils`);
+const { allFilesMatching, resolvePathFor, errorDetailOf } = require(`${libDir}/utils`);
 const { I18nMessage } = require(`${libDir}/i18n`);
 
 const ui = new ConsoleUI(process, console);
@@ -49,7 +49,7 @@ class Testy {
       try {
         await testRunner.run();
       } catch (err) {
-        ui.exitWithError(I18nMessage.of('error_running_suites'), err.stack);
+        ui.exitWithError(I18nMessage.of('error_running_suites'), errorDetailOf(err));
       }
     });
     testRunner.finish();
@@ -90,7 +90,7 @@ class Testy {
       require(file);
     } catch (err) {
       ui.exitWithError(
-        I18nMessage.of('error_loading_suite', file), err.stack,
+        I18nMessage.of('error_loading_suite', file), errorDetailOf(err),
         I18nMessage.of('feedback_for_error_loading_suite'),
       );
     }