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

Add: Alias for find() and findAll() methods for new Element API. Fixes Issue - #4057 #4067

Closed
wants to merge 5 commits into from

Conversation

its-kunal
Copy link

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

This pull request will fix the issue - #4057

@CLAassistant
Copy link

CLAassistant commented Feb 28, 2024

CLA assistant check
All committers have signed the CLA.

@garg3133
Copy link
Member

@its-kunal This isn't what we want. We don't need to create multiple files of the same command to create aliases. The whole concept or motive behind having aliases is so that we can use the same command under different names without repeating the code for all the names.

You'd need to explore the code more and see how can we associate different command names with the same code file.

@garg3133 garg3133 marked this pull request as draft February 28, 2024 17:27
@its-kunal
Copy link
Author

OK, is there any example of how to create aliases? It would be a great help.

@its-kunal
Copy link
Author

@garg3133 I have a question, where do we need to define the definition of alias methods?

@its-kunal
Copy link
Author

Hey @garg3133, I have removed extra files from the pull request. Also please help me where to add definitions of alias methods.

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.

@its-kunal You're almost there.

Two things:

  • Please test your changes by using these commands in one of Nightwatch's example tests (present in examples/tests directory). The following commands does not seem to be working: browser.element.findElements() and browser.element.getAll().
  • Please revert back all the unrelated changes from types/web-element.d.ts file, this PR should only contain the changes that are related to this issue.

@its-kunal
Copy link
Author

Hey @garg3133, Thanks for the feedback. I have made the changes that were asked.

  • Now all tests are passing.
  • Revert backed all the unrelated changes to types/web-element.d.ts

@its-kunal its-kunal marked this pull request as ready for review March 5, 2024 12:12
@dikwickley
Copy link
Contributor

hey @its-kunal there are still changes in the file types/web-element.d.ts
single quotes ' are replaced by double quotes " . According to the lint rules, it should be ' only

@its-kunal
Copy link
Author

Hey @dikwickley i have done the changes as asked.

@its-kunal
Copy link
Author

Hey @dikwickley any more changes required for this issue

@dikwickley
Copy link
Contributor

Can you try adding some more tests that test this alias?

@its-kunal
Copy link
Author

Hey @dikwickley, I think tests for aliases are already present in the file test\src\api\commands\web-element\testFindAll.js, test\src\api\commands\web-element\testFind.js.

@its-kunal
Copy link
Author

Hey @garg3133, @dikwickley, do i need to do any changes to this PR.

@piyushmishra1416
Copy link

Hello @its-kunal , it would be better if you add some tests also for the newly added commands. You can take reference from this PR #4107.

@garg3133
Copy link
Member

@its-kunal Did you follow what I said in this comment? I don't see any implementation for browser.element.findElements() and browser.element.getAll() commands, which means that you have not even verified if your PR actually solves the issue.

@garg3133
Copy link
Member

Fixed in #4130, thanks!

@garg3133 garg3133 closed this Apr 18, 2024
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.

5 participants