-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
(feat): Added aliases for find() and findAll() command in new Element API #4130
Conversation
@AritraLeo You don't need to push the changes you've made in Also, I don't see |
@garg3133 I'll surely revert all the changes from ecosia but before I push changes you can check lines - 166 & 196 where I've tested - Thank You! |
@garg3133 reverted the changes of ecosia.js file tests |
@AritraLeo That's not what I meant. You should test Please see the issue description for all the commands that should work. |
Apologies for the oversight I'll make it work appropriately this time! |
@garg3133 Thank You! |
Please add tests to show the API in use so we can make sure we know it's working as intended. |
Actually I wrote the tests in ecosia.js but I later I came to know that, I should not push the changes there. So I did revert the file and I'll try to write all the tests in - ( Thank You! |
@AritraLeo The aliases for the For the unit tests, as you correctly mentioned, you'd need add those in the |
Thank You so much for the direction @garg3133 I wrote the unit test suite in local but was getting errors it was clear to me that I was missing something I looked into similar merged PR's but couldn't get a solid way to get it resolved. I'll come up with the changes and update UT files asap. Thanks again! |
Hey @garg3133 I am having some queries regarding the tests to be performed for the given -
is this the test (is already present in testFind.js line -70)
cuz after adding the aliases like -
to the file -
it's failing is it because selector needs to accept an id ? |
@AritraLeo Your test is failing because you are not passing a valid selector to the |
I was thinking of this exact thing but last time you asked me to follow the TODO list so I did it anyway. I was pretty sure Thanks again! |
@garg3133 I've added the tests (ran the testing) please have a look also added the alias declaration in element file. |
@garg3133 @AutomatedTester Please review my code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now, thanks!
…ightwatchjs#4130) Co-authored-by: Priyansh Garg <[email protected]>
I have performed the given TODO's for the issue - #4057
Raising this PR to hopefully get the issue resolved. Please let me know in case any change/s is required.
Thank You!