Open Anilkumar-Shrestha opened 2 weeks ago
Visit the deploys page to approve it
Name | Link |
---|---|
Latest commit | a1fede3ae19c73e12216502bc2aef5a936f9ac12 |
β±οΈ Estimated effort to review: 3 π΅π΅π΅βͺβͺ |
π§ͺ PR contains tests |
π No security concerns identified |
β‘ Key issues to review Code Duplication There is some code duplication in the test methods, particularly in setting up the driver, navigating to the page, and handling alerts. Consider extracting common setup code to a separate method or using @Before annotations. Incomplete Assertion In the `promptDefaultInputTest` method, there's a comment indicating that implementation is needed to check if the default value is accepted. This test is incomplete and should be fully implemented. |
Category | Suggestion | Score |
Enhancement |
Implement the check for default value acceptance in promptDefaultInputTest___ **In thepromptDefaultInputTest , implement the check for the default value being accepted as mentioned in the comment.** [examples/java/src/test/java/dev/selenium/interactions/AlertsTest.java [84-85]](https://github.com/SeleniumHQ/seleniumhq.github.io/pull/1900/files#diff-2ec5c2e3450ee03308bd3c130e9b33f0b8696c49c90334028c8e11e8ff6ce356R84-R85) ```diff alert.accept(); -// Implementation needed to check teh default value is accepted. +WebElement result = driver.findElement(By.id("prompt-with-default-result")); +Assertions.assertEquals("Default value", result.getText()); ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9Why: Implementing the check for default value acceptance addresses a missing implementation noted in the comment, improving the completeness and accuracy of the test. | 9 |
Add verification for alert closure after accepting it___ **Consider adding a check to verify if the alert is closed after accepting it, toensure the action was successful.** [examples/java/src/test/java/dev/selenium/interactions/AlertsTest.java [39-41]](https://github.com/SeleniumHQ/seleniumhq.github.io/pull/1900/files#diff-2ec5c2e3450ee03308bd3c130e9b33f0b8696c49c90334028c8e11e8ff6ce356R39-R41) ```diff Alert alert = driver.switchTo().alert(); Assertions.assertEquals("cheese", alert.getText()); alert.accept(); +Assertions.assertThrows(NoAlertPresentException.class, () -> driver.switchTo().alert()); ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: Adding a check to verify if the alert is closed after accepting it enhances the robustness of the test by ensuring that the alert handling was successful. | 8 | |
Add verification for switching back to default content after handling alerts in iframes___ **Consider adding a check to verify if the driver has successfully switched back tothe default content after handling alerts in iframe tests.** [examples/java/src/test/java/dev/selenium/interactions/AlertsTest.java [157]](https://github.com/SeleniumHQ/seleniumhq.github.io/pull/1900/files#diff-2ec5c2e3450ee03308bd3c130e9b33f0b8696c49c90334028c8e11e8ff6ce356R157-R157) ```diff alert.accept(); +driver.switchTo().defaultContent(); +Assertions.assertEquals("Alerts", driver.getTitle()); ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: Verifying that the driver has successfully switched back to the default content after handling alerts in iframes ensures that subsequent tests are not affected by the iframe context, enhancing test reliability. | 8 | |
Best practice |
Use a constant for timeout duration in WebDriverWait initialization___ **Consider using a constant for the timeout duration in the WebDriverWaitinitialization to improve maintainability.** [examples/java/src/test/java/dev/selenium/interactions/AlertsTest.java [23]](https://github.com/SeleniumHQ/seleniumhq.github.io/pull/1900/files#diff-2ec5c2e3450ee03308bd3c130e9b33f0b8696c49c90334028c8e11e8ff6ce356R23-R23) ```diff -wait = new WebDriverWait(driver, Duration.ofSeconds(10)); +private static final Duration TIMEOUT = Duration.ofSeconds(10); +wait = new WebDriverWait(driver, TIMEOUT); ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: Using a constant for the timeout duration improves code maintainability and readability by making it easier to update the timeout value in the future. | 7 |
@harsha509 Can you please guide me on the failed check case _Run Java examples / tests (windows, nightly) (pullrequest) . I am getting hard to understand what check it has been failed with the failed log details.
User description
Description
Motivation and Context
I wanted to ensure that users can get real example of using the alerts pop up and they can execute it easily by copying the test files.
Types of changes
Checklist
PR Type
Tests, Documentation
Description
Changes walkthrough π
AlertsTest.java
Add comprehensive alert handling test cases in Java
examples/java/src/test/java/dev/selenium/interactions/AlertsTest.java
alerts.en.md
Update Java alert examples with code references
website_and_docs/content/documentation/webdriver/interactions/alerts.en.md
alerts.ja.md
Update Java alert examples with code references
website_and_docs/content/documentation/webdriver/interactions/alerts.ja.md
alerts.pt-br.md
Update Java alert examples with code references
website_and_docs/content/documentation/webdriver/interactions/alerts.pt-br.md
alerts.zh-cn.md
Update Java alert examples with code references
website_and_docs/content/documentation/webdriver/interactions/alerts.zh-cn.md