Skip to content

waitUntil, if given should return custom error message when condition returns always false#10535

Merged
christian-bromann merged 8 commits intowebdriverio:mainfrom
rashiq231:bug-10495
Jun 12, 2023
Merged

waitUntil, if given should return custom error message when condition returns always false#10535
christian-bromann merged 8 commits intowebdriverio:mainfrom
rashiq231:bug-10495

Conversation

@rashiq231
Copy link
Contributor

@rashiq231 rashiq231 commented Jun 11, 2023

Proposed changes

waitUntil should return a custom error message if given, else should return a default error message when the condition always returns false ( not resolving to falsy)
//: # (Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.)
This change is to resolve an issue reported in the #10495 , where waitUntil doesn't throw a custom error message if given.
With this change, we check for a condition if the given function always returns falsy and has a custom error message.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Checklist

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have added proper type definitions for new commands (if appropriate)

Further comments

Reviewers: @webdriverio/project-committers

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 11, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

I think the best solution is to do the following change in line 80 of packages/webdriverio/src/utils/Timer.ts#L80:

-                 return this._checkCondition(new Error('return value was never truthy'))
+                return this._checkCondition(new Error(TIMEOUT_ERROR))

Then the right timeout message should be thrown as defined in line 50

@rashiq231
Copy link
Contributor Author

updated as per your comment. Could you please verify now?

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@christian-bromann
Copy link
Member

@rashiq231 I think we need to update one more unit test:

 FAIL  packages/webdriverio/tests/utils/Timer.test.ts > timer > promise > should be rejected without promise
AssertionError: expected Error: timeout to match object Error: return value was never truthy
  - Expected  - 1
  + Received  + 1

    Error {
  -   message: 'return value was never truthy',
  +   message: 'timeout',
    }

@rashiq231
Copy link
Contributor Author

okay... I will update the test case for Timer.
But, a little clarification, shouldn't we need to maintain the existing error message - 'return value was never truthy', when ever the function returns false instead of resolving to false?
Shouldn't the timeout error which we updated in the timer should occur when the given time is exhausted?

@christian-bromann
Copy link
Member

The docs implicitly say:

It expects a condition and waits until that condition is fulfilled with a truthy value to be returned.

So there is no reason to have an error message stating the obvious and use the timeout error because that is what happens at the end: the timer times out because no truthy value was returned.

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@christian-bromann christian-bromann added the PR: Bug Fix 🐛 PRs that contain bug fixes label Jun 12, 2023
@christian-bromann christian-bromann merged commit ad938e5 into webdriverio:main Jun 12, 2023
@christian-bromann
Copy link
Member

christian-bromann commented Jun 12, 2023

Congratulations on your first WebdriverIO contribution! This project can't live without the participation of the community. We would love to see more from you, so let us know if we can help to find interesting areas for you to contribute to. Join our Discord channel and reach out to us. We appreciate you 🙏 ❤️

@rashiq231
Copy link
Contributor Author

@christian-bromann , Thank you for your help and support :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: Bug Fix 🐛 PRs that contain bug fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants