From 7b9e595bdbe6d60845d41d3a0da9a693c8edf292 Mon Sep 17 00:00:00 2001 From: Zearin Date: Tue, 1 Mar 2016 11:09:04 -0500 Subject: [PATCH 1/4] Rewrite TestStep (no Promises, readability) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary: - Removes Promises from TestStep - Rewrite TestStep so it’s easier for newcomers to grok, and factor out the complex middle execution to a separate function - Edit tests for TestStep accordingly ## Details Like the planned removal of EventEmitter, Promises make microcuke harder to understand and less porting-friendly for people using it to write an implementation. This hopefully makes microcuke the TestStep class easier to understand, faster. --- lib/cucumber/test_step.js | 89 +++++++++++++++++---------------- test/cucumber/test_step_test.js | 86 ++++++++----------------------- 2 files changed, 68 insertions(+), 107 deletions(-) diff --git a/lib/cucumber/test_step.js b/lib/cucumber/test_step.js index 512e0f4..106daf0 100644 --- a/lib/cucumber/test_step.js +++ b/lib/cucumber/test_step.js @@ -1,48 +1,51 @@ module.exports = function TestStep(gherkinLocations, matchedArguments, bodyFn, bodyLocation) { - this.execute = function (world, eventEmitter, run) { - eventEmitter.emit('step-started', { - status: 'unknown', - gherkinLocation: gherkinLocations[gherkinLocations.length - 1], - bodyLocation: bodyLocation, - matchedArguments: matchedArguments - }); - // We're returning a promise of a boolean here, because JavaScript. - // Most languages should simply return a boolean and forget about - // the promise stuff - simpler! - return new Promise(function (resolve) { - if (!bodyFn) { - resolve({status: 'undefined'}); - } else if (!run) { - resolve({status: 'skipped'}); - } else { - var matchedArgumentValues = matchedArguments.map(function (arg) { - return arg.value - }); - try { - // Execute the step definition body - var promise = bodyFn.apply(world, matchedArgumentValues); + // this object tracks the execution status as it goes + var data = { + status: 'unknown', + gherkinLocation: gherkinLocations[gherkinLocations.length - 1], + bodyLocation: bodyLocation, + matchedArguments: matchedArguments + } + + // handles execution details + var _execute = function (world, data) { + var matchedArgumentValues = matchedArguments.map( + function (arg) {return arg.value} + ); + try { + // Execute the step definition body + result = bodyFn.apply(world, matchedArgumentValues); + data.status = 'passed'; + } catch (err) { + data.status = 'failed'; + data.error = err; + } finally { + return data; + } + } + + // main execution logic + // + // Overview: + // 1. startup + // 2. execution + // 3. completion + this.execute = function(world, eventEmitter, run) { + // 1. startup + eventEmitter.emit('step-started', data); + + // 2. execution + if (!bodyFn) { + data.status = 'undefined'; + } else if (!run) { + data.status = 'skipped'; + } else { + data = _execute(world, data); + } - if (promise && typeof(promise.then) === 'function') { - // ok, it's a promise - promise.then(function () { - resolve({status: 'passed'}); - }).catch(function (err) { - resolve({status: 'failed', error: err}); - }) - } else { - resolve({status: 'passed'}); - } - } catch (err) { - resolve({status: 'failed', error: err}); - } - } - }).then(function (event) { - event.gherkinLocation = gherkinLocations[gherkinLocations.length - 1]; - event.bodyLocation = bodyLocation; - event.matchedArguments = matchedArguments; - eventEmitter.emit('step-finished', event); - return event.status === 'passed'; - }); + // 3. completion + eventEmitter.emit('step-finished', data); + return data.status === 'passed'; } }; diff --git a/test/cucumber/test_step_test.js b/test/cucumber/test_step_test.js index fade065..d26e947 100644 --- a/test/cucumber/test_step_test.js +++ b/test/cucumber/test_step_test.js @@ -10,34 +10,26 @@ describe("TestStep", function () { }); var eventEmitter = new EventEmitter(); - var step; eventEmitter.on('step-started', function (_step) { - step = _step; + assert.equal(_step.status, 'unknown'); }); var world = {}; testStep.execute(world, eventEmitter, true); - assert.equal(step.status, 'unknown'); }); - it("fires an event with status=failed when returning a rejected promise", function () { + it("fires an event with status=failed when a test does not pass", function () { var locations = []; var testStep = new TestStep(locations, [], function () { - return Promise.reject(Error("sad trombone")); + throw Error("sad trombone"); }); var eventEmitter = new EventEmitter(); - var step; eventEmitter.on('step-finished', function (_step) { - step = _step; + assert.equal(_step.status, 'failed') }); - var world = {}; - return testStep.execute(world, eventEmitter, true) - .then(function (run) { - assert(!run); - assert.equal(step.status, 'failed'); - }); + testStep.execute(world, eventEmitter, true); }); it("fires an event with status=failed when an exception is thrown", function () { @@ -47,17 +39,12 @@ describe("TestStep", function () { }); var eventEmitter = new EventEmitter(); - var step; eventEmitter.on('step-finished', function (_step) { - step = _step; + assert.equal(_step.status, 'failed'); }); var world = {}; - return testStep.execute(world, eventEmitter, true) - .then(function (run) { - assert(!run); - assert.equal(step.status, 'failed'); - }); + return testStep.execute(world, eventEmitter, true); }); it("fires an event with status=skipped when the run parameter is false", function () { @@ -67,56 +54,33 @@ describe("TestStep", function () { }); var eventEmitter = new EventEmitter(); - var step; eventEmitter.on('step-finished', function (_step) { - step = _step; + assert.equal(_step.status, 'skipped'); }); var world = {}; - return testStep.execute(world, eventEmitter, false) - .then(function (run) { - assert(!run); - assert.equal(step.status, 'skipped'); - }); + return testStep.execute(world, eventEmitter, false); }); it("fires an event with status=passed when no exception is thrown", function () { var locations = []; var testStep = new TestStep(locations, [], function () { + var a = 0; + return a }); var eventEmitter = new EventEmitter(); - var step; - eventEmitter.on('step-finished', function (_step) { - step = _step; - }); - - var world = {}; - return testStep.execute(world, eventEmitter, true) - .then(function (run) { - assert(run); - assert.equal(step.status, 'passed'); - }); - }); - - it("fires an event with status=passed when returning a resolved promise", function () { - var locations = []; - var testStep = new TestStep(locations, [], function () { - return Promise.resolve(); - }); - - var eventEmitter = new EventEmitter(); - var step; eventEmitter.on('step-finished', function (_step) { - step = _step; + try { assert.equal(_step.status, 'passed'); } + catch (err) { + console.error(err) + console.error('step was:\n') + console.error(_step) + } }); var world = {}; - return testStep.execute(world, eventEmitter, true) - .then(function (run) { - assert(run); - assert.equal(step.status, 'passed'); - }); + testStep.execute(world, eventEmitter, true); }); it("fires an event with status=undefined when no body exists", function () { @@ -124,17 +88,13 @@ describe("TestStep", function () { var testStep = new TestStep(locations, [], null); var eventEmitter = new EventEmitter(); - var step; eventEmitter.on('step-finished', function (_step) { step = _step; + assert.equal(step.status, 'undefined', true); }); var world = {}; - return testStep.execute(world, eventEmitter, true) - .then(function (run) { - assert(!run); - assert.equal(step.status, 'undefined', true); - }); + testStep.execute(world, eventEmitter, true); }); it("passes argument values to body function", function () { @@ -148,10 +108,8 @@ describe("TestStep", function () { var eventEmitter = new EventEmitter(); var world = {}; - return testStep.execute(world, eventEmitter, true) - .then(function (run) { - assert.equal(arg, 'hello'); - }); + testStep.execute(world, eventEmitter, true); + assert.equal(arg, 'hello'); }); }); }); From 24777e15e1718b8e01bae72e6e6658933516eb25 Mon Sep 17 00:00:00 2001 From: Zearin Date: Tue, 1 Mar 2016 11:35:30 -0500 Subject: [PATCH 2/4] Remove Promises from SequentialTestCaseExecutor Making microcuke simpler to grok as a reference implementation. --- lib/cucumber/sequential_test_case_executor.js | 20 ++++++++++++++----- .../sequential_test_case_executor_test.js | 8 ++------ 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/lib/cucumber/sequential_test_case_executor.js b/lib/cucumber/sequential_test_case_executor.js index bda786b..a02e3f0 100644 --- a/lib/cucumber/sequential_test_case_executor.js +++ b/lib/cucumber/sequential_test_case_executor.js @@ -1,12 +1,22 @@ module.exports = function SequentialTestCaseExecutor(testCases) { this.execute = function (eventEmitter) { + var all_passed = true; + for (var i=0 ; i Date: Tue, 1 Mar 2016 12:02:34 -0500 Subject: [PATCH 3/4] Remove Promises from TestCase Making microcuke simpler to grok as a reference implementation. --- lib/cucumber/test_case.js | 38 +++++++++++++++---------- test/cucumber/test_case_test.js | 49 ++++++++++++++++----------------- 2 files changed, 46 insertions(+), 41 deletions(-) diff --git a/lib/cucumber/test_case.js b/lib/cucumber/test_case.js index 3415159..9ab7d18 100644 --- a/lib/cucumber/test_case.js +++ b/lib/cucumber/test_case.js @@ -7,24 +7,32 @@ module.exports = function TestCase(pickle, testSteps) { pickleSteps: pickle.steps }); var world = {}; - - return runTestStepsInSequence(testSteps, eventEmitter, world) - .then(function () { - eventEmitter.emit('scenario-finished', { - path: pickle.path, - location: location - }); - }); - }; + runTestStepsInSequence(testSteps, eventEmitter, world); + eventEmitter.emit('scenario-finished', { + path: pickle.path, + location: location + } + ); + } function runTestStepsInSequence(testSteps, eventEmitter, world) { + var all_passed = true; + for (var i=0 ; i Date: Tue, 1 Mar 2016 12:17:09 -0500 Subject: [PATCH 4/4] Remove Promises from Glue tests 24777e15e1718b8e01bae72e6e6658933516eb25 --- test/cucumber/glue_test.js | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/test/cucumber/glue_test.js b/test/cucumber/glue_test.js index 4fd7f91..964411a 100644 --- a/test/cucumber/glue_test.js +++ b/test/cucumber/glue_test.js @@ -20,7 +20,7 @@ describe("Glue", function () { return { execute: function () { executed = true; - return Promise.resolve(); + return true; } }; } @@ -31,9 +31,8 @@ describe("Glue", function () { var testCase = glue.createTestCase(pickle); assert(!executed); - return testCase.execute(new EventEmitter()).then(function () { - assert(executed); - }); + testCase.execute(new EventEmitter()); + assert(executed); }); it("creates an undefined step when no stepdefs match", function () { @@ -53,10 +52,8 @@ describe("Glue", function () { assert.equal(step.status, 'undefined'); assert.deepEqual(step.gherkinLocation, {path: 'features/hello.feature', line: 3, column: 11}); }); - return testCase.execute(eventEmitter) - .then(function () { - assert.ok(finished); - }); + testCase.execute(eventEmitter); + assert(finished); }); it("throws an exception when two stepdefs match", function () { @@ -91,7 +88,7 @@ describe("Glue", function () { return { execute: function () { result.push('step'); - return Promise.resolve(); + return true; } }; } @@ -104,8 +101,8 @@ describe("Glue", function () { createTestStep: function (pickle) { return { execute: function () { - result.push('before') - return Promise.resolve(); + result.push('before'); + return; } }; } @@ -116,7 +113,7 @@ describe("Glue", function () { return { execute: function () { result.push('after'); - return Promise.resolve(); + return; } }; } @@ -127,9 +124,8 @@ describe("Glue", function () { var testCase = glue.createTestCase(pickle); assert.deepEqual(result, []); - return testCase.execute(new EventEmitter()).then(function () { - assert.deepEqual(result, ['before', 'step', 'after']); - }); + testCase.execute(new EventEmitter()); + assert.deepEqual(result, ['before', 'step', 'after']); }); }); });