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

Suppressing error for further chained .find() commands #4119

Merged

Conversation

dikwickley
Copy link
Contributor

@dikwickley dikwickley commented Mar 12, 2024

Fixes: #4079

Fixes: dikwickley/gsoc24#1

Solves the third point from this comment (#3991 (comment))

Further chained .find() commands should not throw the Timeout or any other error again. Ex. in browser.element('.invalid-selector').find('selector').getText();, there should only be one error due to browser.element() and .find() should not throw an error again (suppress the error for it).

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.

  • Create a new branch from master (e.g. features/my-new-feature or issue/123-my-bugfix);
  • If you're fixing a bug also create an issue if one doesn't exist yet;
  • If it's a new feature explain why do you think it's necessary. Please check with the maintainers beforehand to make sure it is something that we will accept. Usually we only accept new features if we feel that they will benefit the entire community;
  • Please avoid sending PRs which contain drastic or low level changes. If you are certain that the changes are needed, please discuss them beforehand and indicate what the impact will be;
  • If your change is based on existing functionality please consider refactoring first. Pull requests that duplicate code will most likely be ignored;
  • Do not include changes that are not related to the issue at hand;
  • Follow the same coding style with regards to spaces, semicolons, variable naming etc.;
  • Always add unit tests - PRs without tests are most of the times ignored.

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!.

@dikwickley
Copy link
Contributor Author

@garg3133 is this a good solution to suppress the error for chained find commands

@dikwickley dikwickley changed the title Suppressing error for further chained find commands Suppressing error for further chained .find() commands Mar 12, 2024
@AutomatedTester
Copy link
Member

Can you add a test to show that this works as anticipated

@chikara1608
Copy link
Contributor

Hey, Have you tried the solution on some test cases ?
I ran this over a test case which is
browser.element.find('.invalid_selector').getProperty('innerHTML');

In my case it still didn't suppress the further errors.

[Testing Chain Commands] Test Suite
──────────────────────────────────────────────────────────────────────
ℹ Connected to GeckoDriver on port 4444 (1150ms).
Using: firefox (124.0) on MAC.

Running sample test for debug:
───────────────────────────────────────────────────────────────────────────────────────────────────
Error
Timed out while waiting for element "By(css selector, .invalid_selector)" to be present for 5000 milliseconds.

Error location:
/Users/arjunchikara/Desktop/nightwatch/lib/api/web-element/scoped-element.js:336
––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––
 334 | function createNarrowedError({error, condition, timeout}) {
 335 |   return error.name === 'TimeoutError'
 336 |     ? new Error(`Timed out while waiting for element "${condition}" to be present for ${timeout} milliseconds.`) 
 337 |     : error;
 338 | }
––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––

Error   Error while running .getElementProperty() protocol action: The element with the reference null is not known in the current browsing context

FAILED: 1 errors (5.089s)

@dikwickley
Copy link
Contributor Author

hey @chikara1608
The example you posted is not a further find command, there is only one single find.
see this #3991 (comment)

try it for such an example

browser.element('.invalid_selector').find('.some_other_selector')

@dikwickley
Copy link
Contributor Author

Can you add a test to show that this works as anticipated

hey @AutomatedTester how can I add such test case that checks for the errors thrown.
can you point to some test that checks for the error thrown (I tried looking in web-element tests but couldn't find)

@chikara1608
Copy link
Contributor

ohh my bad! So if I got this right, the solution would basically suppress the error thrown by find in case there was an error in the command before it right ?
But it would still throw error in the command after the find command (for now at-least) right ?
For ex : browser.element('.invalid-selector').find('selector').getText();
This test case throws error at .element as well as .getText but suppresses the find error.

@dikwickley
Copy link
Contributor Author

ohh my bad! So if I got this right, the solution would basically suppress the error thrown by find in case there was an error in the command before it right ? But it would still throw error in the command after the find command (for now at-least) right ? For ex : browser.element('.invalid-selector').find('selector').getText(); This test case throws error at .element as well as .getText but suppresses the find error.

Yes correct

Copy link
Member

@garg3133 garg3133 left a comment

Choose a reason for hiding this comment

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

The solution looks good but could be converted to a single if statement:

      if (parentElement?.webElement && await parentElement.webElement === null) {
        return null;
      }

For tests, we could probably check if the HTTP request for the subsequent find commands goes through (they shouldn't), or if the error messages are logged for all the chained find commands or just the first command. You could try thinking in these lines.

lib/api/web-element/scoped-element.js Outdated Show resolved Hide resolved
@dikwickley dikwickley requested a review from garg3133 August 1, 2024 03:19
@garg3133 garg3133 merged commit d09efe9 into nightwatchjs:main Oct 17, 2024
16 of 17 checks passed
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.

Resolving Comments on Existing PRs Supress error for chained .find() command
4 participants