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

Added isPresent command #4060

Closed
wants to merge 12 commits into from

Conversation

piyushmishra1416
Copy link

Fixes: #4055

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

@CLAassistant
Copy link

CLAassistant commented Feb 26, 2024

CLA assistant check
All committers have signed the CLA.

@piyushmishra1416 piyushmishra1416 changed the title Issue/4055 Added isPresent command Feb 26, 2024
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.

Whenever you propose some code changes, please test them out thoroughly using Nightwatch's example tests.

lib/api/web-element/commands/isPresent.js Show resolved Hide resolved
Comment on lines 587 to 591
async isElementPresent(webElementOrId) {
const element = this.getWebElement(webElementOrId);

return element instanceof WebElement || element instanceof ShadowRoot;
},
Copy link
Member

Choose a reason for hiding this comment

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

Did you test the command yourself by using it in some example tests. I feel that this would not work in case the element does not exist.

@piyushmishra1416
Copy link
Author

Hello @garg3133 I thoroughly checked my code for isPresent command but yes as you have said testIsPresent.js is failing the tests where the element is not present. Also, I have checked isPresent command with some example test cases provided like duckduckgo.js and it is working fine with those tests. Could you please review my PR now and provide me some valuable feedback?

@piyushmishra1416
Copy link
Author

Hello @garg3133 , just wanted to follow up on my previous request for a review. I understand you're busy, but I would appreciate your feedback whenever you have a chance. Thank you!

@garg3133
Copy link
Member

garg3133 commented Mar 1, 2024

Also, I have checked isPresent command with some example test cases provided like duckduckgo.js and it is working fine with those tests.

Did you check with invalid selectors as well if the command is working in that case?

@garg3133
Copy link
Member

garg3133 commented Mar 1, 2024

I tried running your PR locally and it is returning true even in the case of an invalid selector and moreover it is throwing an error.

image

@@ -583,6 +584,21 @@ module.exports = class MethodMappings {
return value;
},

async isElementPresent(webElementOrId) {
try {
const element = this.getWebElement(webElementOrId);
Copy link
Member

Choose a reason for hiding this comment

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

The problem here is that if you use VSCode Debugger and come here, you'll notice that webElementOrId is actually a WebElementPromise (pay attention to 'Promise' here), which means it is not yet certain if the result after the promise is resolved would even exist or not.

So, you may try to resolve that promise first to see the result and then match that result with WebElement or ShadowRoot.

After doing that, please try to run the command in one of example tests (and by example tests, I mean the tests present in examples/tests directory) and see if the command is actually working as expected.

Copy link
Author

Choose a reason for hiding this comment

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

Sure I will fix this soon.

Copy link
Author

@piyushmishra1416 piyushmishra1416 Mar 2, 2024

Choose a reason for hiding this comment

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

I have tried to update the method-mappings and got the result as false when an element is not present but still, I am finding this timeouterror. I guess isPresent searches an element for a time period of 5000ms and then give out false if it does not find an element. So to fix this what would be the correct approach? `

  • OTHER ERRORS:
    Error
    Error
    Timed out while waiting for element "By(css selector, input[name="z"])" to be present for 5000 milliseconds.`
image

Comment on lines 18 to 24
MockServer.addMock({
url: '/session/13521-10219-202/element',
method: 'POST',
response: JSON.stringify({
value: {ELEMENT: '0'} // Simulate element found
})
}, true);
Copy link
Member

Choose a reason for hiding this comment

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

This mock is not useful because that URL is never hit.

To see all the request/response happening behind the scenes, run the test as VERBOSE=1 npx mocha test/src/api/commands/web-element/testIsPresent.js.

Copy link
Author

Choose a reason for hiding this comment

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

How to get more information about the mockserver? I will try to refer this as you have suggested in other PR.

Copy link
Member

Choose a reason for hiding this comment

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

MockServer is mostly fairly simple, read this: https://discord.com/channels/618399631038218240/1070690424185966662/1210478133371871243.

It just intercepts the calls going on a particular URL and returns the response provided by us.

Copy link
Author

@piyushmishra1416 piyushmishra1416 Mar 2, 2024

Choose a reason for hiding this comment

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

Hello @garg3133 , this is the verbose output I am getting after removing the mock servers.
`
element().isPresent() command
⠋ Connecting to localhost on port 10195...
Request POST /session
{
capabilities: { firstMatch: [ {} ], alwaysMatch: { browserName: 'firefox' } }
}
Response 201 POST /session (13ms)
{
value: {
sessionId: '13521-10219-202',
capabilities: {
acceptInsecureCerts: false,
browserName: 'firefox',
browserVersion: '65.0.1',
platformName: 'linux',
platformVersion: '4.9.125-linuxkit',
setWindowRect: true,
strictFileInteractability: false,
timeouts: { implicit: 0, pageLoad: 300000, script: 30000 },
unhandledPromptBehavior: 'dismiss and notify'
}
}
ℹ Connected to localhost on port 10195 (19ms).
Using: firefox (65.0.1) on LINUX (4.9.125-linuxkit).

Received session with ID: 13521-10219-202

→ Running command: element.find ()
Request POST /session/13521-10219-202/elements
{ using: 'css selector', value: '#signupSection' }
Response 200 POST /session/13521-10219-202/elements (1ms)
{ value: [ { 'element-6066-11e4-a52e-4f735466cecf': '0' } ] }
→ Completed command: element.find () (2ms)

→ Running command: element().isElementPresent ()
✔ test .element().isPresent() for element present
→ Completed command: element().isElementPresent () (0ms)

→ Running command: element.find ()
Request POST /session/13521-10219-202/elements
{ using: 'css selector', value: '#signupSecti' }
Response 404 POST /session/13521-10219-202/elements (1ms)

UnsupportedOperationError
findElements:
→ Completed command: element.find () (2ms)

→ Running command: element().isElementPresent ()
✔ test .element().isPresent() for element not present
→ Completed command: element().isElementPresent () (0ms)

→ Running command: element.find ()
Request POST /session/13521-10219-202/elements
{ using: 'css selector', value: '#signupSection' }
Response 200 POST /session/13521-10219-202/elements (0ms)
{ value: [ { 'element-6066-11e4-a52e-4f735466cecf': '0' } ] }
→ Completed command: element.find () (2ms)

→ Running command: element().isElementPresent ()
✔ test async .element().isPresent() for element present
→ Completed command: element().isElementPresent () (0ms)

→ Running command: element.find ()
Request POST /session/13521-10219-202/elements
{ using: 'css selector', value: '#badElement' }
Response 404 POST /session/13521-10219-202/elements (0ms)

UnsupportedOperationError
findElements:
→ Completed command: element.find () (2ms)

→ Running command: element().isElementPresent ()
✔ test .element().isPresent() handles errors
→ Completed command: element().isElementPresent () (0ms)`

Copy link
Author

Choose a reason for hiding this comment

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

Hello @garg3133 , isPresent is working fine after removing the mock servers so do we actually need mock servers for the isPresent command or should I remove this completely? I have taken reference from hereand they are also not using mock servers. Also, can you please review my PR?

@piyushmishra1416
Copy link
Author

Hello @garg3133, could you please review this pull request? Also, if there are any issues, could you kindly let me know what I am missing and any modifications that can be made to enhance this PR?

@@ -583,6 +584,21 @@ module.exports = class MethodMappings {
return value;
},

async isElementPresent(webElementOrId) {
try {
const element = await this.getWebElement(webElementOrId);
Copy link

Choose a reason for hiding this comment

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

@piyushmishra1416 this method will be used with new element apis i.e. the argument will always be a web element and not the id. considering this can you please rename the argument name to webElement from webElementOrId and additionally, update the const element = await this.getWebElement(webElementOrId) to const element = await webElement

Please add a comment on the top of the function regarding this assumption

Please address the changes in the test cases accordingly. Comment back here once you are done with the changes and the testing

Copy link
Author

Choose a reason for hiding this comment

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

fixed this

Copy link

Choose a reason for hiding this comment

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

Please add a comment on the top of the function regarding this assumption
can you please address this as well

did you try running test cases again?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, and it was working fine for the elements that were present.

@ansh21
Copy link

ansh21 commented Mar 29, 2024

@piyushmishra1416 please resolve the conflicts

@ansh21
Copy link

ansh21 commented Apr 3, 2024

I tried running your PR locally and it is returning true even in the case of an invalid selector and moreover it is throwing an error.

image

@piyushmishra1416 Can you please address this comment as well, this is still replicable
fyi @garg3133

@piyushmishra1416
Copy link
Author

Hello @ansh21, thank you for reviewing the PR. I apologize for the delayed response as I was preoccupied with other tasks. I have made the necessary updates as per your suggestion.

@piyushmishra1416
Copy link
Author

Also @ansh21 as I have mentioned here comment, I am still finding the timeout error. Is it because it is a chained command as mentioned in this issue ? Please provide me with the correct approach to resolve this.

@ansh21
Copy link

ansh21 commented Apr 8, 2024

finding the timeout error. Is

@garg3133 can you please address this?

@garg3133
Copy link
Member

isPresent command was added in #4216, closing this PR. Thanks a lot for your efforts on this PR, they're much appreciated. And sorry that we couldn't get this PR to the final stage.

@garg3133 garg3133 closed this Sep 13, 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.

Add missing isPresent command in new Element API
4 participants