Add isPresent command.
#4216
Add isPresent command.
#4216
Conversation
Status
|
|
There are some failing tests. Not sure if it is due to the changes made here. Can you help take a look? Thank you! |
|
@pujagani yes, fixed those. There was a missing check for outofbounds index. |
|
I think the test that is failing is flakey. I can't see a test for this change can we add one? |
|
@garg3133 @AutomatedTester ready for review! |
| if (args[0]?.suppressNotFoundErrors) { | ||
| this._suppressNotFoundErrors = true; | ||
| } | ||
|
|
There was a problem hiding this comment.
Can be moved to the top of function.
| @@ -172,7 +176,7 @@ class ScopedWebElement { | |||
|
|
|||
| return await this.findElement({parentElement, selector: condition, index, timeout, retryInterval}); | |||
| } catch (error) { | |||
| if (suppressNotFoundErrors) { | |||
| if (this.suppressNotFoundErrors ?? suppressNotFoundErrors) { | |||
There was a problem hiding this comment.
No need to have two suppressNotFoundErrors, just keep the this.suppressNotFoundErrors.
If suppressNotFoundErrors comes out to be true here, set this._suppressNotFoundErrors to true and then no need to pass the suppressNotFoundErrors property in this.findElementAction.
| } catch (error) { | ||
| if (error.name === 'NoSuchElementError') { | ||
| return false; // Element does not exist | ||
| } | ||
| throw error; // Re-throw other errors for further handling | ||
| } |
There was a problem hiding this comment.
In what cases will this catch block be hit?
Because the webElement in the new Element API is designed to never actually throw an error in normal cases (including when element is not found), I don't think try-catch is required here. For other errors, they would just be thrown directly.
Is there any case that I am missing here in which this catch block will be useful?
| const common = require('../../../../common.js'); | ||
| const Element = common.require('element/index.js'); | ||
|
|
||
| describe('element().isPresent() command', function() { |
There was a problem hiding this comment.
These tests are passing even if we set the suppressNotFoundErrors property to false in the isPresent.js file. We must also include at least one test here which fails in such a case to make sure that our unit tests are testing what they are supposed to test, that we do not throw an error in case of isPresent() command.
Thanks in advance for your contribution. Please follow the below steps in submitting a pull request, as it will help us with reviewing it quicker.
The isPresent command was quite tricky as there were few edge cases we had to solve.
The main one being when the element in not present, we don't want to show the Timeout Error if there is a isPresent command chained (which should ideally return false).
I solved this by checking if the next sibling of a
findcommand is anelementIsPresentcommand. It still takes 5 seconds to try to resolve the element.If this approach looks good, I can add some tests for this too.
Sanity for elements that are not present:
examples/testsdirectory of the project) and running them.ecosia.jsandduckDuckGo.jsare good examples to work with.features/my-new-featureorissue/123-my-bugfix);