Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Continue to run hooks and close session on uncaught errors. #4269

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/api/_loaders/_base-loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,7 @@ class BaseLoader extends EventEmitter {

//prevent unhandled rejection.
node.deferred.promise.catch(err => {
BaseLoader.lastDeferred.reject(err);
BaseLoader.lastDeferred?.reject(err);
});

if (!this.module?.returnFn) {
Expand Down
4 changes: 2 additions & 2 deletions lib/api/_loaders/assertion.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class AssertionLoader extends BaseCommandLoader {

static validateAssertClass(instance) {
Object.keys(AssertionLoader.interfaceMethods).forEach(method => {
let type = AssertionLoader.interfaceMethods[method];
const type = AssertionLoader.interfaceMethods[method];
if (!BaseCommandLoader.isTypeImplemented(instance, method, type)) {
const methodTypes = method.split('|').map(name => `"${name}"`);

Expand Down Expand Up @@ -113,7 +113,7 @@ class AssertionLoader extends BaseCommandLoader {

// prevent unhandledRejection errors
node.deferred.promise.catch(err => {
return AssertionLoader.lastDeferred.reject(err);
return AssertionLoader.lastDeferred?.reject(err);
});

return node.deferred.promise;
Expand Down
2 changes: 1 addition & 1 deletion lib/api/_loaders/static.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ module.exports = class StaticAssert {

//prevent unhandled rejection
node.deferred.promise.catch(err => {
return StaticAssert.lastDeferred.reject(err);
return StaticAssert.lastDeferred?.reject(err);
});

return node.deferred.promise;
Expand Down
2 changes: 1 addition & 1 deletion lib/api/web-element/scoped-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ class ScopedWebElement {
// which results in the command result to be `null` in test case as well, while the command actually failed.
// Is this expected? To return `null` result in case of failure as well?
// eslint-disable-next-line no-prototype-builtins
return result.hasOwnProperty('value') ? result.value : result;
return result?.hasOwnProperty('value') ? result.value : result;
};

return node;
Expand Down
8 changes: 7 additions & 1 deletion lib/core/queue.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,13 @@ class CommandQueue extends EventEmitter {
return node;
}

this.tree.addNode(node);
if (!process.env.NW_UNCAUGHT_ERROR) {
this.tree.addNode(node);
} else {
setTimeout(() => {
node.resolve(null);
});
}

if (this.currentNode.done || !this.currentNode.started || initialChildNode) {
this.scheduleTraverse();
Expand Down
83 changes: 76 additions & 7 deletions lib/runner/process-listener.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const {Logger} = require('../utils');
const {Logger, SafeJSON} = require('../utils');
const analyticsCollector = require('../utils/analytics.js');

module.exports = class {
Expand Down Expand Up @@ -56,7 +56,7 @@ module.exports = class {
this.uncaught(err, {type: 'unhandledRejection'});
}

getCurrentPromise(err) {
async getCurrentPromise(err) {
if (this.testRunner) {
const {currentSuite} = this.testRunner;

Expand All @@ -67,25 +67,94 @@ module.exports = class {
}

this.testRunner.registerUncaughtErr(err);

// let testSuiteReportResults;
if (currentSuite) {
// if (currentSuite.commandQueue) {
// // empty the queue but keep it in progress, unlike `currentSuite.emptyQueue()` which resets the queue completely.
// // (we'll add `end` command to the queue later so it should continue fine)
// const tree = currentSuite.commandQueue.tree;
// tree.currentNode = tree.rootNode;
// tree.empty();
// }
currentSuite.emptyQueue();
currentSuite.setUncaughtError(err);
// To reject the deferred promise directly inside runnable.js > run(), but this had problems:
// * if the promise is already resolved before, this won't do anything.
// currentSuite.currentRunnable?.deferred?.rejectFn(err);

// make sure the report is up-to-date for events till now.
const reporter = currentSuite.reporter;
reporter.registerTestError(err);

// below lines should be kept for old way (without setting env and closing the process)
// currentSuite.testCaseFinished();
// testSuiteReportResults = reporter.exportResults();

// await currentSuite.client.api.end(true, () => {
// // empty queue for any command added while the `.end()`
// // command was executing (async event loop).
// currentSuite.emptyQueue();
// });

// const endCommandPromise = currentSuite.client.api.end(true, () => {
// // empty queue for any command added while the `.end()`
// // command was executing (async event loop).
// process.env.NW_UNCAUGHT_ERROR = '1';
// });

const quitCommandPromise = currentSuite.client.api.quit();
process.env.NW_UNCAUGHT_ERROR = '1';

await quitCommandPromise;
}


// await this.testRunner.closeOpenSessions(); // could've used in place of `.end()` call above but no mechanism to clear queue again with this.

if (this.testRunner.publishReport) {
this.testRunner.publishReport = false;
this.testRunner.reportResults().then(() => {}).catch(err => console.error(err));
// since we are now continuing the execution, we should not publish the report here.
// so, commenting out the below lines.

// this.testRunner.publishReport = false;

// // see how uncaught error can be added to the report.
// // console.log(testSuiteReporter.exportResults());
// // this.testRunner.globalReporter.addTestSuiteResults(testSuiteReportResults);

// this.testRunner.printGlobalResults();


// The code below is for passing the report to the main thread in case of uncaught errors in parallel processes.

// return this.testRunner.reportResults().catch(err => console.error(err)).then(() => {
// if (this.testRunner.isTestWorker() && process.port && typeof process.port.postMessage === 'function') {
// // const reporter = testSuiteReporter;
// // reporter.registerFailed(err);

// process.port.postMessage(SafeJSON.stringify({
// type: 'testsuite_finished',
// results: testSuiteReportResults, // could be undefined??
// httpOutput: Logger.collectOutput()
// }));
// }
// });
}
}
}

uncaught(err, {type = 'uncaughtException'} = {}) {
const isVerboseLogEnabled = Logger.isEnabled();
Logger.setOutputEnabled(true); // force log for uncaught exception
Logger.enable();

Logger.error(`${type}: ${err.message}\n${err.stack}`);
analyticsCollector.collectErrorEvent(err, true);

if (!isVerboseLogEnabled) {
Logger.disable();
}

if (['TimeoutError', 'NoSuchElementError'].includes(err.name) && this.testRunner.type !== 'cucumber') {
this.closeProcess(err);
if (this.testRunner && this.testRunner.publishReport) {
Expand All @@ -96,9 +165,9 @@ module.exports = class {
return;
}

this.getCurrentPromise(err);

return this.closeProcess(err);
return this.getCurrentPromise(err).then(() => {
// return this.closeProcess(err);
});
}

closeProcess(err) {
Expand Down
1 change: 1 addition & 0 deletions lib/runner/test-runners/cucumber.js
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ class CucumberSuite extends TestSuite {
testSuiteFinished() {}

async runTestSuite() {
// return here too if `process.env.NW_UNCAUGHT_ERROR` is set???
let result;
try {
result = await this.cucumberCli.run();
Expand Down
4 changes: 4 additions & 0 deletions lib/runner/test-runners/default.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ class DefaultRunner {
}

async runTestSuite(modulePath, modules) {
if (process.env.NW_UNCAUGHT_ERROR) {
return;
}

try {
this.currentSuite = await this.createTestSuite({modulePath, modules});
} catch (err) {
Expand Down
9 changes: 7 additions & 2 deletions lib/testsuite/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -798,7 +798,7 @@ class TestSuite {
runNextTestCase() {
const nextTestCase = this.context.getNextKey();

if (nextTestCase) {
if (nextTestCase && !process.env.NW_UNCAUGHT_ERROR) {
return this.runCurrentTest(nextTestCase);
}

Expand Down Expand Up @@ -859,6 +859,10 @@ class TestSuite {
.then(() => possibleError);
})
.then(possibleError => {
if (process.env.NW_UNCAUGHT_ERROR) {
throw possibleError || 'uncaughtException/unhandledRejection encountered';
}

if (this.shouldRetryTestCase()) {
return this.retryCurrentTestCase();
}
Expand Down Expand Up @@ -1052,7 +1056,8 @@ class TestSuite {
}
// clearing the queue here to avoid continuing with the rest of the testcase,
// unless abortOnFailure is set to false
if (!this.isAssertionError(err) || err.abortOnFailure) {
if (!process.env.NW_UNCAUGHT_ERROR && (!this.isAssertionError(err) || err.abortOnFailure)) {
// if `process.env.NW_UNCAUGHT_ERROR` is set, nothing will be in the queue except for the quit command.
this.emptyQueue();
}

Expand Down
4 changes: 4 additions & 0 deletions lib/testsuite/retries.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ class SuiteRetries {
return false;
}

if (process.env.NW_UNCAUGHT_ERROR) {
return false;
}

this.testRetriesCount[testName] = this.testRetriesCount[testName] || 0;

return this.testRetriesCount[testName] < this.testMaxRetries;
Expand Down
24 changes: 22 additions & 2 deletions lib/testsuite/runnable.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,8 @@ class Runnable {
}

setDoneCallback(cb) {
let originalResolve = this.deffered.resolveFn;
let originalReject = this.deffered.rejectFn;
const originalResolve = this.deffered.resolveFn;
const originalReject = this.deffered.rejectFn;
this.deffered.resolveFn = function() {};
this.deffered.rejectFn = function() {};

Expand All @@ -123,6 +123,8 @@ class Runnable {
let result;

try {
// the result below represents whatever is returned from the `it` function
// (a promise if `it` is async otherwise whatever is actually returned).
result = this.runFn();
} catch (err) {
this.deffered.rejectFn(err);
Expand All @@ -134,6 +136,9 @@ class Runnable {
const deferredFn = () => {
result
.then(res_ => {
// if there is already a queue in progress, it will be resolved after the current queue is done.
// but if there is no queue in progress, we can't be sure if the `it` block block would ever be resolved
// (could be stuck forever) so we resolve it here.
if (this.queueInProgress()) {
return;
}
Expand All @@ -147,6 +152,21 @@ class Runnable {

if (this.isES6Async) {
// the timeout is needed for situations where there is an async function without any await commands
// (because otherwise the `result` would resolve immediately without queue even being started, leading to
// `this.deferred` being resolved before the queue is started). Also, what if the `it` block is async but
// there is no Nightwatch command in it -- then also the `it` block should get resolved.
// Two ways of resolving/completing a test case: either this or calling `done` after the queue is completely traversed.
//
// when we empty the queue in process-listener and a new queue is created, since the new queue is not started
// yet, the deferred promise resolves above, leading to starting of `afterEach` hook.
// we should avoid the above.

// maybe instead of resetting the tree, just empty the tree and reset to the root node while keeping it in progress.
// (but this could also have unknown side effects)
//
// or maybe just let it be and let the afterEach hook run and put its command in the same queue, because it should anyway
// be rare to have another queue made after emptying it since most of the time the uncaught error would be from within a
// command and not from within the `it` function directly. (Read note from page "Requirements (uncaught..Rejection")
setTimeout(() => deferredFn(), 20);

// without .catch() here, we can get an unhandledRejection
Expand Down
Loading