-
Notifications
You must be signed in to change notification settings - Fork 9
Extra Windows Helpers #2585
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
Extra Windows Helpers #2585
Conversation
| int winCount = getDriver().getWindowHandles().size(); | ||
| link.sendKeys(Keys.chord(WebDriverUtils.MODIFIER_KEY, Keys.ENTER)); | ||
| if (getDriver().getWindowHandles().size() < (winCount + 1)) | ||
| throw new IllegalStateException("Link did not open new window in tab."); | ||
|
|
||
| switchToWindow(1); | ||
| waitForDocument(); |
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.
switchToWindow already waits for the new window. However, this should be updated to switch to the new window instead of always using index 1.
| int winCount = getDriver().getWindowHandles().size(); | |
| link.sendKeys(Keys.chord(WebDriverUtils.MODIFIER_KEY, Keys.ENTER)); | |
| if (getDriver().getWindowHandles().size() < (winCount + 1)) | |
| throw new IllegalStateException("Link did not open new window in tab."); | |
| switchToWindow(1); | |
| waitForDocument(); | |
| int winCount = getDriver().getWindowHandles().size(); | |
| link.sendKeys(Keys.chord(WebDriverUtils.MODIFIER_KEY, Keys.ENTER)); | |
| switchToWindow(winCount); | |
| waitForDocument(); |
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.
So I'm trying to test all anchor tags that could function like links, whether they have a href or a click event handler attached later. And there are many anchor tags on our pages that are not links. So I'm using this function to determine if an anchor tag is a link by trying to click on it and see if a new window pops up. If so, it's a real link. If not, it throws which is caught in the crawler test and it moves on to the next anchor tag. I incorporated your suggestion above, but also created a new function, openLinkInNewWindowOrThrow more designed for this use case.
| public void openLinkInNewWindowOrThrow(WebElement link) | ||
| { |
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 method is redundant. switchToWindow will throw if the specified window index doesn't exist. That being said, I wouldn't hate if it threw a more informative error (currently, you'd just get an IndexOutOfBoundsException)
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.
yeah switchToWindow waits 10 secs before throwing that IndexOutOfBoundsException. Since we're using this as a test to discover anchor tags that are links, there will be a relative high number of failures. So openLinkInNewWindowOrThrow speeds up that check for this use case and throws a more explicit exception. If you think it's ok to refactor openLinkInNewWindow to more quickly check and throw then we could use it here without slowing down this test.
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.
That's still a pretty misleading method name. Maybe just add timeout parameters to openLinkInNewWindow and switchToWindow
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.
Ok, that would be a pretty big refactor, switchToWindow is used in a lot of places. Maybe I'm pushing too hard to use this general functionality for a specific use case it doesn't quite match. I'll close this PR and do a more focused function in ehrModules.
| public void openLinkInNewWindowOrThrow(WebElement link) | ||
| { |
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.
That's still a pretty misleading method name. Maybe just add timeout parameters to openLinkInNewWindow and switchToWindow
|
Closing this PR for more specific implementation in the test. |
Rationale
Test helper functions for EHR link crawler test.
Related Pull Requests
Changes