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

Fix waitUntil not working with async/await. #4123

Conversation

chikara1608
Copy link
Contributor

@chikara1608 chikara1608 commented Mar 13, 2024

Fixes: #3511.

waitUntil command was getting prematurely resolved when used with async/await due to which the execution of the callback passed to the waitUntil command was getting overlapped with the Nightwatch commands following the waitUntil command.

This was happening because inside the command queue tree, whenever child nodes are resolved, the parent node is also automatically called to be resolved which was causing pre mature resolution of waitUntil.

@CLAassistant
Copy link

CLAassistant commented Mar 13, 2024

CLA assistant check
All committers have signed the CLA.

@@ -189,7 +189,7 @@ class AsyncTree extends EventEmitter{
}

const isAssertion = node.parent && (node.parent.namespace === 'assert' || node.parent.namespace === 'verify');
if (node.parent && !node.parent.isRootNode && !isAssertion && !stillInProgress) {
if (node.parent && !node.parent.isRootNode && !isAssertion && !stillInProgress && node.parent.name !== 'waitUntil') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't ideally hardcode a command's name over here, but instead have some logic inside this.otherChildNodesInProgress() method to decide whether the command has finished completely or there are still some child nodes that can potentially be added to the command, again without hardcoding the command name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if the waitUntil command fails, we should ideally abort the test execution and display the error instead of continuing the test execution.

await browser.waitUntil().pause() would do just that (if the waitUntil command fails, it won't execute the pause() command.

But if we write it as:

await browser.waitUntil();
browser.pause();

it would execute the pause() command even after the waitUntil command fails.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What we can do here is (for the first comment) declare a flag in the command file itself (as you suggested earlier) using a get method and then use that flag to decide whether to resolve the parent node as well with the same result or not.

This can be done by returning the flag value from getCommandOptions() method in lib/api/_loaders/_command-loader.js file, which will then be available in node.options in the asynctree.js file and can be used there.

And instead of using this.otherChildNodesInProgress() for this purpose, we can create a new this.resolveParentNodeWithResult() method and put the logic for deciding this in there.

Copy link
Member

@garg3133 garg3133 Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And then maybe we can iteratively do this for all the commands to deprecate this logic altogether (as this logic I think is only relevant to the client-commands which then call some protocol command to do the work, as we're slowly deprecating such commands).

I think a good way to do this would be to bypass this logic for a few commands in every release to ensure that we don't end up breaking anything (and if we get complain for some commands, we can just revert the commit from that release). And once we've bypassed this logic for all the commands, we can safely remove this.

For the note, according to my understanding, this logic was used for commands that call other commands to do the actual work, and when those "child-commands" returned a result, the parent command (which is the original command that the user used) should also be resolved with the same result, so we resolve the parent node as well with the same result as the child node using this logic.

But we don't need to do any of this in the current PR, this PR is only for waitUntil command.

lib/api/protocol/waitUntil.js Outdated Show resolved Hide resolved
lib/api/_loaders/_command-loader.js Outdated Show resolved Hide resolved
lib/core/asynctree.js Outdated Show resolved Hide resolved
@AutomatedTester AutomatedTester linked an issue Mar 19, 2024 that may be closed by this pull request
lib/core/asynctree.js Outdated Show resolved Hide resolved
lib/core/asynctree.js Show resolved Hide resolved
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 Issue/3511 patched a fix for waitUntil to work with async await Fix waitUntil not working with async/await. Mar 19, 2024
@garg3133
Copy link
Member

@AutomatedTester This PR is ready to be merged.

There are a few issues related to waitUntil command still left, for which we might need to file separate GitHub issues:

@AutomatedTester AutomatedTester merged commit b08981d into nightwatchjs:main Mar 21, 2024
16 of 17 checks passed
@dikwickley dikwickley mentioned this pull request Mar 21, 2024
9 tasks
@garg3133
Copy link
Member

garg3133 commented Mar 27, 2024

There are a few issues related to waitUntil command still left, for which we might need to file separate GitHub issues:

Opened #4157 and #4158 for the first two issues.

For the third issue, it must have something to do with the side effects of the queuing system arising from the waitUntil callback getting overlapped with the other commands, due to which the result of some other command must be getting passed to the getText command (maybe the getText command became a parent to some other command and then got resolved prematurely with the result of a child command).

It won't be possible (or easy) to investigate the issue without the verbose logs so we can leave it for now. Anyways, as the user said, it gets resolved after removing the waitUntil command, so it must be happening due to the callback overlapping only, which is resolved now.

Related Discord thread for the third issue: https://discord.com/channels/618399631038218240/1085703830685093918/1085703830685093918

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

waitUntil not working with async/await
4 participants