Repository: nightwatchjs/nightwatch
Pull request #4325 (2024-12-08 21:34:48)

Fix asynctree from getting cleared in between test case.
## Problem

Right now, if a test case is written using the `async/await` syntax, the `asynctree` we use inside our queueing system gets reset after every `await`ed Nightwatch command/assertion, after which the `done()` method of the queue itself is called.

But, in between the tree getting reset and the queue's `done()` method being called, the control goes back inside the test case itself (because the `await`ed command/assertion is resolved now) and it reads the following NW commands and adds them into a new queue asynctree. So, when the `done()` method of the queue is called after that, because of [this condition](https://github.com/nightwatchjs/nightwatch/blob/44ba57c2604b81f76bae7d620236a744e99b41d3/lib/core/queue.js#L124-L126) which checks if the tree is empty or not, because the tree is non-empty now, nothing happens and the test case continues its normal execution.

But, when the pointer goes back to the test case between the tree getting reset and the queue `done()` call, instead of executing a NW command (which results in the queue tree getting filled again), if we execute a normal JS code and `await` it (like the `setTimeout` code below):
```
await browser.waitForElementVisible('body');

await new Promise((resolve) => {
  setTimeout(function() {
    resolve(null);
  }, 50);
});

await browser
  .assert.visible('input[name=qi]')
  .sendKeys('input[name=q', 'nightwatch');
```
the tree will stay empty after the execution of this code and the control will pass to the `done()` method of the queue.

The above situation will cause the current queue to be resolved (through the call to [`this.deferred.resolve()`](https://github.com/nightwatchjs/nightwatch/blob/44ba57c2604b81f76bae7d620236a744e99b41d3/lib/core/queue.js#L135)), which in turn will cause the current runnable's [`this.deferred.resolveFn(result)`](https://github.com/nightwatchjs/nightwatch/blob/44ba57c2604b81f76bae7d620236a744e99b41d3/lib/testsuite/runnable.js#L167) to be called. But because it is called with the `result` passed as argument, which represents the promise returned by the currently running test case, the current runnable does not actually get resolved but remains stuck waiting for the test case to complete and the `result` to be resolved.

This means that because the resolution of the current runnable is now tied to the resolution of the promise returned by the test case, the current runnable will never be resolved until the test case itself is completed and resolved.

---

Now, when the awaited JS code inside the test case (due to which all of the above happened) finally gets resolved and returns its result, the following NW commands/assertions will be executed and added to the same queue (but with a different [`this.deferred`](https://github.com/nightwatchjs/nightwatch/blob/44ba57c2604b81f76bae7d620236a744e99b41d3/lib/core/queue.js#L38) no longer tied to the current runnable) and everything will continue running normally.

So, if nothing eventful happens and the rest of the test case executes successfully, the `result` to which the resolution of the current runnable is tied to will resolve normally, leading to the resolution of the current runnable and everything works fine.

BUT, if there is some error or an assertion failure someplace in the remaining test case, the tree will get cleared [here](https://github.com/nightwatchjs/nightwatch/blob/44ba57c2604b81f76bae7d620236a744e99b41d3/lib/core/asynctree.js#L170-L175) [, followed by the entry inside the test case which is covered below and not important here], followed by a call to the `done()` method of the queue and hence the call to queue's [`this.deferred.resolve()`](https://github.com/nightwatchjs/nightwatch/blob/44ba57c2604b81f76bae7d620236a744e99b41d3/lib/core/queue.js#L135). But since the `this.deferred` of the queue is no longer tied to the current runnable, this path of the resolution/rejection of current runnable is closed now.

Now, the resolution/rejection of the current runnable depends entirely on the resolution/rejection of the test case itself.

So, if this error or failure in the currently running NW command/assertion leads to:
* rejection of the test case (will happen if `await` is directly used with the command and the command promise gets rejected)
  ```js
  await browser
    .click()
    .failingCommand();  // `await` is directly tied to the promise returned by the `failingCommand`.
  ```
  --> the `result` to which the resolution of the current runnable is tied will get rejected and the current runnable will resolve with an error.
  --> TEST WILL CONTINUE NORMALLY.
* continuation of the test case (will happen if `await` is directly used with the command and the command promise gets resolved)
  ```js
  await browser
    .click()
    .failingCommand();  // `await` is directly tied to the promise returned by the `failingCommand`.

  await browser.sendKeys();
  ```
  --> the test case will continue after the `failingCommand` and run the `.sendKeys` command.
  --> TEST WILL CONTINUE BUT ABNORMALLY.
* the test case getting stuck (will happen if the command is chained with some other NW commands)
  ```js
  await browser
    .click()
    .failingCommand()  // `await` is not tied to the promise returned by the `failingCommand`.
    .sendKeys();
  ```
  --> the test case will get stuck after the `failingCommand` becasue the tree is emptied after the failure and so the last `sendKeys` command (to which the `await` is tied) will never run or get resolved.
  --> TEST WILL GET STUCK OR THE PROCESS WILL EXIT ABRUPTLY.


## SOLUTION

The root cause of this issue is that if a test case is written with the async/await syntax, the tree is reset after every awaited NW command/assertion, which is then followed by a call to the queue `done()` method. Instead of this, the tree should also be reset at the end of the current test case or hook (or in case there is an error or a failure in some command, which is covered [here](https://github.com/nightwatchjs/nightwatch/blob/44ba57c2604b81f76bae7d620236a744e99b41d3/lib/core/asynctree.js#L170-L175)). On doing this, although the queue `done()` method will still be called after every awaited NW command/assertion, it will [never be executed completely](https://github.com/nightwatchjs/nightwatch/blob/44ba57c2604b81f76bae7d620236a744e99b41d3/lib/core/queue.js#L124-L126) because the tree will be non-empty at that point.

This also sovles an issue when using the "Step over and pause again" feature of the `.pause()` command where the feature wasn't working for test cases written with async/await syntax.


github-actions[bot]: 
## Status
* ❌ No modified files found in the **types** directory.
Please make sure to include types for any changes you have made. Thank you!.

Patch:
diff --git a/lib/api/_loaders/command.js b/lib/api/_loaders/command.js
index da1ac057..ee961116 100644
--- a/lib/api/_loaders/command.js
+++ b/lib/api/_loaders/command.js
@@ -151,6 +151,18 @@ class CommandLoader extends BaseCommandLoader {
 
   createWrapper() {
     if (this.module) {
+      // this place is only reached by client-commands, protocol commands and custom-commands (no assertions or element-commands).
+      if (this.isUserDefined) {
+        // only custom-commands will reach here.
+        // later extend this to client-commands and protocol commands as well.
+        Object.defineProperty(this.module, 'rejectNodeOnAbortFailure', {
+          configurable: true,
+          get() {
+            return true;
+          }
+        });
+      }
+
       this.commandFn = function commandFn({args, stackTrace}) {
         const instance = CommandLoader.createInstance(this.nightwatchInstance, this.module, {
           stackTrace,
diff --git a/lib/core/asynctree.js b/lib/core/asynctree.js
index 926a45a1..abf85cde 100644
--- a/lib/core/asynctree.js
+++ b/lib/core/asynctree.js
@@ -106,7 +106,7 @@ class AsyncTree extends EventEmitter{
   shouldRejectNodePromise(err, abortOnFailure, node = this.currentNode) {
     const rejectNodeOnAbortFailure = node.options?.rejectNodeOnAbortFailure;
 
-    if ((err.isExpect || node.namespace === 'assert' || (abortOnFailure && rejectNodeOnAbortFailure)) && this.currentNode.isES6Async) {
+    if ((err.isExpect || node.namespace === 'assert' || (abortOnFailure && rejectNodeOnAbortFailure)) && node.isES6Async) {
       return true;
     }
 
@@ -134,7 +134,6 @@ class AsyncTree extends EventEmitter{
     this.emit('asynctree:command:start', {node});
 
     const result = await node.run();
-    const {parent} = this.currentNode;
 
     let abortOnFailure = false;
     let err;
@@ -155,7 +154,7 @@ class AsyncTree extends EventEmitter{
       }
 
       if (this.shouldRejectParentNodePromise(err, node)) {
-        parent.reject(err);
+        node.parent.reject(err);
       }
     } else {
       node.resolveValue = result;
@@ -212,6 +211,15 @@ class AsyncTree extends EventEmitter{
   }
 
   async done(err = null) {
+    // `this.currentTestCaseResult` represents the return value of the
+    // currently executing `it` test case or hook.
+    // We do not want to clear the tree if the test case is still running.
+    // In case of an error, the tree will be cleared in the `runChildNode`
+    // method itself.
+    if (this.currentTestCaseResult instanceof Promise && !this.currentTestCaseResult.settled) {
+      return err;
+    }
+
     this.emit('asynctree:finished', this);
     this.empty();
     this.createRootNode();
diff --git a/lib/core/queue.js b/lib/core/queue.js
index 3e8b3a71..eac2f339 100644
--- a/lib/core/queue.js
+++ b/lib/core/queue.js
@@ -145,7 +145,8 @@ class CommandQueue extends EventEmitter {
     return Promise.resolve();
   }
 
-  run() {
+  run(currentTestCaseResult) {
+    this.tree.currentTestCaseResult = currentTestCaseResult;
     if (this.tree.started) {
       return this;
     }
diff --git a/package-lock.json b/package-lock.json
index 1a8598c8..a76d8457 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -37,7 +37,7 @@
         "open": "8.4.2",
         "ora": "5.4.1",
         "piscina": "^4.3.1",
-        "selenium-webdriver": "4.26.0",
+        "selenium-webdriver": "4.27.0",
         "semver": "7.5.4",
         "stacktrace-parser": "0.1.10",
         "strip-ansi": "6.0.1",
@@ -8098,10 +8098,19 @@
       "dev": true
     },
     "node_modules/selenium-webdriver": {
-      "version": "4.26.0",
-      "resolved": "https://registry.npmjs.org/selenium-webdriver/-/selenium-webdriver-4.26.0.tgz",
-      "integrity": "sha512-nA7jMRIPV17mJmAiTDBWN96Sy0Uxrz5CCLb7bLVV6PpL417SyBMPc2Zo/uoREc2EOHlzHwHwAlFtgmSngSY4WQ==",
-      "license": "Apache-2.0",
+      "version": "4.27.0",
+      "resolved": "https://registry.npmjs.org/selenium-webdriver/-/selenium-webdriver-4.27.0.tgz",
+      "integrity": "sha512-LkTJrNz5socxpPnWPODQ2bQ65eYx9JK+DQMYNihpTjMCqHwgWGYQnQTCAAche2W3ZP87alA+1zYPvgS8tHNzMQ==",
+      "funding": [
+        {
+          "type": "github",
+          "url": "https://github.com/sponsors/SeleniumHQ"
+        },
+        {
+          "type": "opencollective",
+          "url": "https://opencollective.com/selenium"
+        }
+      ],
       "dependencies": {
         "@bazel/runfiles": "^6.3.1",
         "jszip": "^3.10.1",
diff --git a/package.json b/package.json
index e9c2f0b1..b62be523 100644
--- a/package.json
+++ b/package.json
@@ -43,7 +43,7 @@
     "open": "8.4.2",
     "ora": "5.4.1",
     "piscina": "^4.3.1",
-    "selenium-webdriver": "4.26.0",
+    "selenium-webdriver": "4.27.0",
     "semver": "7.5.4",
     "stacktrace-parser": "0.1.10",
     "strip-ansi": "6.0.1",
diff --git a/types/web-element.d.ts b/types/web-element.d.ts
index 6505b388..76f7d65b 100644
--- a/types/web-element.d.ts
+++ b/types/web-element.d.ts
@@ -360,10 +360,9 @@ export type ScopedElementRect = {
   readonly height: number;
 };
 
-export type DragAndDropDestination = {
-  readonly xOffset: number;
-  readonly yOffset: number;
-};
+export type DragAndDropDestination = 
+  | {readonly x: number; readonly y: number;}
+  | WebElement
 
 export interface ElementFunction
   extends Pick<


Test patch:
diff --git a/lib/testsuite/context.js b/lib/testsuite/context.js
index 9a9b4f3f..f12e9715 100644
--- a/lib/testsuite/context.js
+++ b/lib/testsuite/context.js
@@ -525,7 +525,12 @@ class Context extends EventEmitter {
       args[0] = client.api;
     }
 
-    return this.testsuite[fnName].apply(context, args);
+    const result = this.testsuite[fnName].apply(context, args);
+    if (this.currentRunnable) {
+      this.currentRunnable.currentTestCaseResult = result;
+    }
+
+    return result;
   }
 
   /**
@@ -541,7 +546,12 @@ class Context extends EventEmitter {
     const fnAsync = Utils.makeFnAsync(expectedArgs, this.testsuite[fnName], context);
     const args = this.getHookFnArgs(done, api, expectedArgs);
 
-    return fnAsync.apply(context, args);
+    const result = fnAsync.apply(context, args);
+    if (this.currentRunnable) {
+      this.currentRunnable.currentTestCaseResult = result;
+    }
+
+    return result;
   }
 
   ////////////////////////////////////////////////////////////////
diff --git a/lib/testsuite/runnable.js b/lib/testsuite/runnable.js
index b60f43d3..bb42d03e 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() {};
 
@@ -159,7 +159,32 @@ class Runnable {
       }
     }
 
-    this.queue.run().then(err => {
+    // `this.currentTestCaseResult` represents the return value of the currently
+    // running test case or hook.
+    // in case the runnable is executing something other than a test case/hook,
+    // `this.currentTestCaseResult` will be `undefined`.
+    if (this.currentTestCaseResult instanceof Promise) {
+      this.currentTestCaseResult
+        .catch(() => {
+          // to avoid unhandledRejections
+          // although the test case promises are already handled for rejections
+          // above (`result.catch()`), if we don't use `.catch()` here again,
+          // `.finally` will return a new promise that will be rejected without
+          // any error handling.
+        })
+        .finally(() => {
+          // mark the promise as settled as a cue to the asynctree so that it
+          // can get cleared out and subsequently call the queue `done` method.
+          this.currentTestCaseResult.settled = true;
+
+          // sometimes this promise is settled after the last call of
+          // asynctree `done` method, so we need schedule a tree traversal
+          // again to clear out the tree and call the queue `done` method.
+          this.queue.scheduleTraverse();
+        });
+    }
+
+    this.queue.run(this.currentTestCaseResult).then(err => {
       if (err) {
         return this.deffered.rejectFn(err);
       }
diff --git a/test/extra/commands/customCommandWithFailureClass.js b/test/extra/commands/customCommandWithFailureClass.js
new file mode 100644
index 00000000..5c869a6c
--- /dev/null
+++ b/test/extra/commands/customCommandWithFailureClass.js
@@ -0,0 +1,5 @@
+module.exports = class CustomCommandWithFailureClass{
+  async command() {
+    await this.api.waitForElementPresent('#badElement', 100);
+  }
+};
diff --git a/test/sampletests/withcustomcommands/sampleWithAsyncFailures.js b/test/sampletests/withcustomcommands/sampleWithAsyncFailures.js
new file mode 100644
index 00000000..c89c227a
--- /dev/null
+++ b/test/sampletests/withcustomcommands/sampleWithAsyncFailures.js
@@ -0,0 +1,15 @@
+module.exports = {
+  before(client) {
+    client.globals.increment++;
+  },
+
+  async demoTestAsync(client) {
+    await client.url('http://localhost').customCommandWithFailureClass();
+    // below statement should be unreachable because the custom command should be rejected
+    client.globals.increment++;
+  },
+
+  after(client) {
+    client.globals.increment++;
+  }
+};
\ No newline at end of file
diff --git a/test/sampletests/withdescribe/basic2/sample.js b/test/sampletests/withdescribe/basic2/sample.js
new file mode 100644
index 00000000..6a03ec49
--- /dev/null
+++ b/test/sampletests/withdescribe/basic2/sample.js
@@ -0,0 +1,80 @@
+const assert = require('assert');
+
+describe('basic describe test', async function () {
+  const nwClient = this['[client]'];
+
+  const asynctreeEventListener = () => {
+    this.globals.asynctreeFinishedEventCount++;
+  };
+
+  const queueEventListener = () => {
+    this.globals.queueFinishedEventCount++;
+  };
+
+  this.desiredCapabilities = {
+    name: 'basicDescribe'
+  };
+
+  test('with awaited JS command bw Nightwatch commands', async function (client) {
+    nwClient.queue.tree.on('asynctree:finished', asynctreeEventListener);
+
+    // queue event listeners are automatically removed after
+    // every (test case + afterEach + after (if applicable))
+    // so, no need to remove this listener manually before next test case run.
+    nwClient.queue.on('queue:finished', queueEventListener);
+
+    await client.assert.strictEqual(client.options.desiredCapabilities.name, 'basicDescribe');
+    assert.equal(nwClient.queue.tree.rootNode.childNodes.length, 1);
+
+    await client.url('http://localhost');
+    assert.equal(nwClient.queue.tree.rootNode.childNodes.length, 2);
+
+    await new Promise((resolve) => {
+      setTimeout(function() {
+        resolve(null);
+      }, 50);
+    });
+    assert.equal(nwClient.queue.tree.rootNode.childNodes.length, 2);
+
+    await client.assert.elementPresent('#weblogin');
+    assert.equal(nwClient.queue.tree.rootNode.childNodes.length, 3);
+
+    // queue:finished event shouldn't be emitted in between the test.
+    assert.equal(this.globals.queueFinishedEventCount, 0);
+  });
+
+  test('with await in last command', async function (client) {
+    nwClient.queue.tree.on('asynctree:finished', asynctreeEventListener);
+
+    await client.assert.strictEqual(client.options.desiredCapabilities.name, 'basicDescribe');
+    assert.equal(nwClient.queue.tree.rootNode.childNodes.length, 1);
+
+    await client.url('http://localhost');
+    assert.equal(nwClient.queue.tree.rootNode.childNodes.length, 2);
+
+    await client.assert.elementPresent('#weblogin');
+    assert.equal(nwClient.queue.tree.rootNode.childNodes.length, 3);
+  });
+
+  test('without await in last command', async function (client) {
+    nwClient.queue.tree.on('asynctree:finished', asynctreeEventListener);
+
+    await client.assert.strictEqual(client.options.desiredCapabilities.name, 'basicDescribe');
+    assert.equal(nwClient.queue.tree.rootNode.childNodes.length, 1);
+
+    await client.url('http://localhost');
+    assert.equal(nwClient.queue.tree.rootNode.childNodes.length, 2);
+
+    client.assert.elementPresent('#weblogin');
+    assert.equal(nwClient.queue.tree.rootNode.childNodes.length, 3);
+  });
+
+  afterEach(function () {
+    nwClient.queue.tree.removeListener('asynctree:finished', asynctreeEventListener);
+    assert.equal(nwClient.queue.tree.rootNode.childNodes.length, 0);
+  });
+
+  after(function (client) {
+    client.end();
+  });
+});
diff --git a/test/src/core/testAsyncTree.js b/test/src/core/testAsyncTree.js
new file mode 100644
index 00000000..676698ca
--- /dev/null
+++ b/test/src/core/testAsyncTree.js
@@ -0,0 +1,47 @@
+const assert = require('assert');
+const path = require('path');
+const CommandGlobals = require('../../lib/globals/commands-w3c.js');
+const common = require('../../common.js');
+const {runTests} = common.require('index.js');
+const {settings} = common;
+
+describe('element().isPresent() command', function() {
+  before(function (done) {
+    CommandGlobals.beforeEach.call(this, done);
+  });
+
+  after(function (done) {
+    CommandGlobals.afterEach.call(this, done);
+  });
+
+  it('test runner with multiple test interfaces - exports/describe', function () {
+    const srcFolders = [
+      path.join(__dirname, '../../sampletests/withdescribe/basic2')
+    ];
+
+    const globals = {
+      asynctreeFinishedEventCount: 0,
+      queueFinishedEventCount: 0,
+      reporter(results, cb) {
+        if (results.lastError) {
+          throw results.lastError;
+        }
+
+        // should equal the number of test cases.
+        assert.equal(globals.asynctreeFinishedEventCount, 3);
+
+        // only counts for the first test case + afterEach
+        assert.equal(globals.queueFinishedEventCount, 2);
+
+        cb();
+      }
+    };
+
+    return runTests(settings({
+      globals,
+      src_folders: srcFolders,
+      output: true,
+      silent: false
+    }));
+  });
+});
diff --git a/test/src/runner/testRunWithCustomCommands.js b/test/src/runner/testRunWithCustomCommands.js
index 79d12676..fcc7956d 100644
--- a/test/src/runner/testRunWithCustomCommands.js
+++ b/test/src/runner/testRunWithCustomCommands.js
@@ -34,14 +34,14 @@ describe('testRunWithCustomCommands', function() {
   });
 
   it('testRunner with custom command which has failures', function() {
-    let testsPath = path.join(__dirname, '../../sampletests/withcustomcommands');
-    let globals = {
+    const testsPath = path.join(__dirname, '../../sampletests/withcustomcommands');
+    const globals = {
       increment: 0,
       retryAssertionTimeout: 0,
       waitForConditionPollInterval: 10,
       waitForConditionTimeout: 20,
       reporter(results, cb) {
-        assert.strictEqual(globals.increment, 4);
+        assert.strictEqual(globals.increment, 6);
         cb();
       }
     };
@@ -55,12 +55,12 @@ describe('testRunWithCustomCommands', function() {
   });
 
   it('testRunner with ES6 Async custom commands', function() {
-    let testsPath = path.join(__dirname, '../../sampletests/withes6asynccommands');
+    const testsPath = path.join(__dirname, '../../sampletests/withes6asynccommands');
     let testResults;
     const origExit = process.exit;
     process.exit = function() {};
 
-    let globals = {
+    const globals = {
       increment: 0,
       logResult: null,
       retryAssertionTimeout: 0,
@@ -101,12 +101,12 @@ describe('testRunWithCustomCommands', function() {
   });
 
   it('testRunner with ES6 Async custom commands', function() {
-    let testsPath = path.join(__dirname, '../../sampletests/withes6asynccommands');
+    const testsPath = path.join(__dirname, '../../sampletests/withes6asynccommands');
     let testResults;
     const origExit = process.exit;
     process.exit = function() {};
 
-    let globals = {
+    const globals = {
       increment: 0,
       logResult: null,
       retryAssertionTimeout: 0,
@@ -147,12 +147,12 @@ describe('testRunWithCustomCommands', function() {
   });
 
   it('testRunner custom command which extends built-in command', function() {
-    let testsPath = path.join(__dirname, '../../sampletests/withcustomcommands/element');
+    const testsPath = path.join(__dirname, '../../sampletests/withcustomcommands/element');
     let testResults;
     const origExit = process.exit;
     process.exit = function() {};
 
-    let globals = {
+    const globals = {
       increment: 0,
       logResult: null,
       retryAssertionTimeout: 0,
@@ -229,12 +229,12 @@ describe('testRunWithCustomCommands', function() {
   });
 
   it('testRunner custom command path as glob pattern', function() {
-    let testsPath = path.join(__dirname, '../../sampletests/withcustomcommands/element');
+    const testsPath = path.join(__dirname, '../../sampletests/withcustomcommands/element');
     let testResults;
     const origExit = process.exit;
     process.exit = function() {};
 
-    let globals = {
+    const globals = {
       increment: 0,
       logResult: null,
       retryAssertionTimeout: 0,
diff --git a/types/tests/webElement.test-d.ts b/types/tests/webElement.test-d.ts
index 6bd10a5e..c0634a29 100644
--- a/types/tests/webElement.test-d.ts
+++ b/types/tests/webElement.test-d.ts
@@ -191,7 +191,8 @@ describe('new element() api', function () {
     expectType<Promise<WebElement>>(elem.submit());
     expectType<Promise<WebElement>>(elem.setProperty('type', 'text'));
     expectType<Promise<WebElement>>(elem.setAttribute('role', 'button'));
-    expectType<Promise<WebElement>>(elem.dragAndDrop({xOffset: 150, yOffset: 500}));
+    expectType<Promise<WebElement>>(elem.dragAndDrop({x: 150, y: 500}));
+    expectType<Promise<WebElement>>(elem.dragAndDrop(elem.webElement));
     expectType<Promise<WebElement>>(elem.moveTo(100, 100));
     expectType<Promise<WebElement>>(elem.clickAndHold());
     expectType<Promise<WebElement>>(elem.doubleClick());