From 92a24ee0200d1fdd207610b12aa0ca3479196e1c Mon Sep 17 00:00:00 2001 From: Priyansh Garg Date: Tue, 24 Sep 2024 15:34:35 +0530 Subject: [PATCH] Continue to run hooks and close session on uncaught errors. --- lib/api/_loaders/_base-loader.js | 2 +- lib/api/_loaders/assertion.js | 4 +- lib/api/_loaders/static.js | 2 +- lib/api/web-element/scoped-element.js | 2 +- lib/core/queue.js | 8 ++- lib/runner/process-listener.js | 83 ++++++++++++++++++++++++--- lib/runner/test-runners/cucumber.js | 1 + lib/runner/test-runners/default.js | 4 ++ lib/testsuite/index.js | 9 ++- lib/testsuite/retries.js | 4 ++ lib/testsuite/runnable.js | 24 +++++++- 11 files changed, 126 insertions(+), 17 deletions(-) diff --git a/lib/api/_loaders/_base-loader.js b/lib/api/_loaders/_base-loader.js index d36abbef91..acaf3cf265 100644 --- a/lib/api/_loaders/_base-loader.js +++ b/lib/api/_loaders/_base-loader.js @@ -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) { diff --git a/lib/api/_loaders/assertion.js b/lib/api/_loaders/assertion.js index 1da40ce7c2..97779a2b4c 100644 --- a/lib/api/_loaders/assertion.js +++ b/lib/api/_loaders/assertion.js @@ -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}"`); @@ -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; diff --git a/lib/api/_loaders/static.js b/lib/api/_loaders/static.js index dc75f485ba..493ce83ce8 100644 --- a/lib/api/_loaders/static.js +++ b/lib/api/_loaders/static.js @@ -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; diff --git a/lib/api/web-element/scoped-element.js b/lib/api/web-element/scoped-element.js index 3ada4a6274..0018c4e250 100644 --- a/lib/api/web-element/scoped-element.js +++ b/lib/api/web-element/scoped-element.js @@ -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; diff --git a/lib/core/queue.js b/lib/core/queue.js index 3e8b3a71bb..600c28876c 100644 --- a/lib/core/queue.js +++ b/lib/core/queue.js @@ -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(); diff --git a/lib/runner/process-listener.js b/lib/runner/process-listener.js index af5784144e..0e7c229660 100644 --- a/lib/runner/process-listener.js +++ b/lib/runner/process-listener.js @@ -1,4 +1,4 @@ -const {Logger} = require('../utils'); +const {Logger, SafeJSON} = require('../utils'); const analyticsCollector = require('../utils/analytics.js'); module.exports = class { @@ -56,7 +56,7 @@ module.exports = class { this.uncaught(err, {type: 'unhandledRejection'}); } - getCurrentPromise(err) { + async getCurrentPromise(err) { if (this.testRunner) { const {currentSuite} = this.testRunner; @@ -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) { @@ -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) { diff --git a/lib/runner/test-runners/cucumber.js b/lib/runner/test-runners/cucumber.js index bb270a63f5..5ecee1e470 100644 --- a/lib/runner/test-runners/cucumber.js +++ b/lib/runner/test-runners/cucumber.js @@ -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(); diff --git a/lib/runner/test-runners/default.js b/lib/runner/test-runners/default.js index bbebe03585..6d20c49712 100644 --- a/lib/runner/test-runners/default.js +++ b/lib/runner/test-runners/default.js @@ -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) { diff --git a/lib/testsuite/index.js b/lib/testsuite/index.js index b20e88b422..bc75fad5be 100644 --- a/lib/testsuite/index.js +++ b/lib/testsuite/index.js @@ -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); } @@ -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(); } @@ -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(); } diff --git a/lib/testsuite/retries.js b/lib/testsuite/retries.js index d8d3c82f80..e4a9ae0818 100644 --- a/lib/testsuite/retries.js +++ b/lib/testsuite/retries.js @@ -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; diff --git a/lib/testsuite/runnable.js b/lib/testsuite/runnable.js index b60f43d3a8..4f756d89f5 100644 --- a/lib/testsuite/runnable.js +++ b/lib/testsuite/runnable.js @@ -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() {}; @@ -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); @@ -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; } @@ -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