-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 missing isSelected
command to new Element API.
#4038
Add missing isSelected
command to new Element API.
#4038
Conversation
@dikwickley Please refer to #4033 (comment) for tests. |
@garg3133 thanks for the review Added the test case as well. Also as for the type of
|
Yeah, sorry.
You can take a reference from there but you need to keep the format of the test similar to what we have in |
Also, can you please sign the CLA? |
Co-authored-by: Priyansh Garg <priyanshgarg30@gmail.com>
Co-authored-by: Priyansh Garg <priyanshgarg30@gmail.com>
hey @garg3133 I have updated the tests and signed the CLA. |
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.
We are nearly there; I would recommend adding a few negative tests as well.
Co-authored-by: Vaibhav Singh <singh.vaibhav2011@gmail.com>
@vaibhavsingh97 let me know if anything else is needed |
there is a test failing for windows (18.x)
|
@dikwickley That's just a flaky test, ignore it. It will automatically pass on a re-run. |
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.
This PR is almost complete, just a feedback on tests:
Like I said before, the format of the test needs to be same as test/src/api/commands/web-element/testGetText.js
because we want to test everything we tested for .getText()
command in .isSelected()
test as well.
So, the test file for isSelected()
should have all the 4 test cases that are present in the test file of getText()
, you just need to replace getText
with isSelected
there along with a bunch of other things like the mocks, how to interpret results, etc.
For example, the first test case will look as the following:
const resultPromise = this.client.api.element('#signupSection').isSelected();
assert.strictEqual(resultPromise instanceof Element, false);
assert.strictEqual(typeof resultPromise.find, 'undefined');
assert.strictEqual(resultPromise instanceof Promise, false);
assert.strictEqual(typeof resultPromise.then, 'function');
const result = await resultPromise;
assert.strictEqual(result instanceof WebElement, false);
assert.strictEqual(result, true);
const resultValue = await resultPromise.value;
assert.strictEqual(resultValue, true);
2cf3ee3
to
f5efa74
Compare
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.
One last bit for test: add a test case to test the .assert
available on the command (see 4th test case of testGetText.js
file).
You only need to text .assert.eqauls
and .assert.not.equals
in this case.
isSelected
to new Element APIisSelected
command to new Element API.
@AutomatedTester This PR is ready to be merged now. |
fixes: #4036
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.
features/my-new-feature
orissue/123-my-bugfix
);