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

#273 throw NoAlertPresentException, NoSuchWindowException, or NoSuchF… #774

Merged
merged 6 commits into from Aug 6, 2018

Conversation

Projects
None yet
4 participants
@tsukakei
Copy link
Contributor

tsukakei commented Aug 4, 2018

…rameException instead of TimeoutException

Proposed changes

Fow now, switchTo().alert() throws TimeoutException as well as switchTo().frame() and switchTo().window() do.
I think these methods should throw NoAlertPresentException, NoSuchFramePresentException, or NoSuchWindowPresentException respectively.

This PR is related to #273.

Checklist

  • Checkstyle and unit tests pass locally with my changes by running gradle check chrome htmlunit command
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
#273 throw NoAlertPresentException, NoSuchWindowException, or NoSuchF…
…rameException instead of TimeoutException
@tsukakei

This comment has been minimized.

Copy link
Contributor

tsukakei commented Aug 4, 2018

To what files should I add some tests?
ErrorMessagesForMissingElementTest is OK, or should I create a new file?

And, can anyone please tell me how to specify integration test classes when executing $ gradle check chrome htmlunit?

@symonk

This comment has been minimized.

Copy link
Contributor

symonk commented Aug 4, 2018

perhaps IT tests around assertsThrownBy to ensure correct exceptions are thrown under the given scenarios, I started on this myself too so thanks for doing this :) I have no time to review it this morning, I need to pop out. Collaborator(s) will come along shortly with some feedback.

@asolntsev
Copy link
Contributor

asolntsev left a comment

@tsukakei Thank you for the pull request!
I would ask for some minor changes.

The easiest way to cover your changes by automated tests is adding another @Test method to AlertTest:

assertThatThrownBy(() -> {
  switchTo().alert();
}).isInstanceOf(NoAlertPresentException.class)
try {
return Wait().until(alertIsPresent());
} catch (TimeoutException e) {
throw new NoAlertPresentException();

This comment has been minimized.

@asolntsev

asolntsev Aug 4, 2018

Contributor

Don't forget to wrap the original exception: throw new NoAlertPresentException(e);

@@ -156,7 +171,7 @@ public WebDriver window(int index) {
return Wait().until(windowToBeAvailableAndSwitchToIt(index));
}
catch (TimeoutException e) {
throw wrapThrowable(e, timeout);
throw new NoSuchWindowException("Window with index not found: " + index);

This comment has been minimized.

@asolntsev

asolntsev Aug 4, 2018

Contributor

Don't forget to wrap the original exception:

throw new NoSuchWindowException("Window with index not found: " + index, e);

tsukakei added some commits Aug 4, 2018

@tsukakei

This comment has been minimized.

Copy link
Contributor

tsukakei commented Aug 6, 2018

Thank you for your instruction and review!
I fixed source code to wrap the original exception and added some tests for the exceptions.

@rosolko

This comment has been minimized.

Copy link
Collaborator

rosolko commented Aug 6, 2018

@tsukakei
Looks good.
Only one point - can you please assert exception with message also. At least in one test.
Just add .hasMessage("<message>") after .isInstanceOf() call.
Just to ensure that right message throws.

@tsukakei

This comment has been minimized.

Copy link
Contributor

tsukakei commented Aug 6, 2018

Thank you for your kind advice! @rosolko
I added assertions to check error messages.

@rosolko rosolko self-requested a review Aug 6, 2018

@rosolko

rosolko approved these changes Aug 6, 2018

Copy link
Collaborator

rosolko left a comment

LGTM

@rosolko rosolko added this to the 4.12.4 milestone Aug 6, 2018

@symonk

This comment has been minimized.

Copy link
Contributor

symonk commented Aug 6, 2018

looks good, thanks for helping out @tsukakei

@asolntsev asolntsev self-assigned this Aug 6, 2018

@asolntsev asolntsev merged commit 816622d into selenide:master Aug 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@asolntsev

This comment has been minimized.

Copy link
Contributor

asolntsev commented Aug 6, 2018

Merged.

@tsukakei @symonk @rosolko Thank you for your effort!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment