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

Reject custom commands node promise on abortOnFailure. #4314

Merged
merged 4 commits into from
Dec 27, 2024

Conversation

garg3133
Copy link
Member

@garg3133 garg3133 commented Nov 27, 2024

If a command results in an error with abortOnFailure set to true, the node promise of the command inside the test case still gets resolved instead of getting rejected, while the queue is anyway cleared because of abortOnFailure.

This leads to a situation where if we chain some more commands after this command in the test case, the test would get stuck (because the queue is emptied) and hence will abort successfully. But, if the await is directly used on the command, that await will resolve and the test will continue normally since the following commands will be added to the queue after the queue is cleared.

The command promise inside the test case should instead be rejected so that the test case throws an error and thus aborting itself instead of continuing normally.

Impact

Only affects custom-commands, which will now reject inside testcase on abort failures instead of getting resolved.

Future TODOs

  • If a command chain ends with an assert command and some command in mid gets failed, the assertion gets resolved, resulting in the testcase continuing instead of getting stuck or failing.
    Moreover, since the queue is emptied on a command failure, the commands between the failed command and resolved assertion never get executed but the commands after resolved assertion will start to run normally.
  • Extend this functionality to all client-commands and protocol commands while making sure that commands like .getTitle() also work correctly if there's an abort failure inside the title() command (child-command of the .getTitle() command).
  • (Low Priority) On failure of a custom-command, done() function of queue is reached before the main command could be resolved, leading to runnable getting resolved/rejected before the testcase is actually wrapped up. So, resolve the custom command treenode in testcase before moving to done() or delay the call of done().
    ^ done() is being called earlier due to the failure of a sub-command of the custom-command.
    The main issue this causes is when inside the test case the custom-command gets resolved (after the call of done() and hence resolution/rejection of the runnable), the following commands in the testcase get added to the queue which then overlap with the commands of further tests (like saveScreenshot, afterEach) because the runnable was settled earlier.
    ^ This main issue should be resolved in this PR because we are now rejecting the custom command node inside the test instead of resolving it.

Note: On the third point, even when this PR solves the issue partially, if the user decides to use try/catch to catch the custom command error inside the testcase, the done() would have still been called earlier due to which the following commands inside the test case will start to overlap with future runnables. So, to fix this fully, we should probably not call done() for the sub-command failure. On doing this, the main command will get rejected in the testcase before the call of queue done() (arising from the custom-command failure), it will be caught and other commands will be added to the queue. This would result in the queue done() call getting skipped in this case because at that point there will be some nodes existing inside the tree.

Copy link

Status

  • ❌ No modified files found in the types directory.
    Please make sure to include types for any changes you have made. Thank you!.

@garg3133 garg3133 changed the title Reject command node promise on abortOnFailure. Reject custom commands node promise on abortOnFailure. Dec 18, 2024
@garg3133 garg3133 merged commit 44ba57c into nightwatchjs:main Dec 27, 2024
16 of 17 checks passed
@garg3133 garg3133 deleted the reject-commands-abortOnFailure branch December 28, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant