Closed pallavigitwork closed 1 week ago
Visit the deploys page to approve it
Name | Link |
---|---|
Latest commit | ab5111cebad3ebe8abdb3bb302440ff6a0c67a32 |
โฑ๏ธ Estimated effort to review: 3 ๐ต๐ต๐ตโชโช |
๐งช PR contains tests |
๐ No security concerns identified |
โก Key issues to review Resource Management The WebDriver instance is not properly closed using try-with-resources or a finally block, which may lead to resource leaks. Test Structure The test method contains multiple assertions and operations, which violates the single responsibility principle for unit tests. Consider splitting into multiple test methods. Hardcoded Wait The use of implicitlyWait with a fixed duration may lead to unreliable tests. Consider using explicit waits for better test stability. |
Category | Suggestion | Score |
Best practice |
Use try-with-resources to automatically close the WebDriver___ **Use a try-with-resources statement to ensure the WebDriver is closed properly, evenif an exception occurs.** [examples/java/src/test/java/dev/selenium/interactions/FramesTest.java [31-71]](https://github.com/SeleniumHQ/seleniumhq.github.io/pull/1915/files#diff-c32bcc5edb4bcccf7c3694e3a6103af9a1bb389f8a7b6afee1821662f87df0baR31-R71) ```diff -WebDriver driver = new ChromeDriver(); -driver.manage().timeouts().implicitlyWait(Duration.ofMillis(500)); +try (WebDriver driver = new ChromeDriver()) { + driver.manage().timeouts().implicitlyWait(Duration.ofMillis(500)); + + // Navigate to Url + driver.get("https://www.selenium.dev/selenium/web/iframes.html"); + + ... + + // No need for explicit quit() call +} -// Navigate to Url -driver.get("https://www.selenium.dev/selenium/web/iframes.html"); - -... - -//quit the browser -driver.quit(); - ``` Suggestion importance[1-10]: 9Why: The suggestion to use try-with-resources ensures that the WebDriver is closed properly, even if an exception occurs, which is a best practice for resource management in Java. | 9 |
Replace implicit wait with explicit wait for better control and reliability___ **Use a WebDriverWait instead of implicit wait to improve test reliability andperformance.** [examples/java/src/test/java/dev/selenium/interactions/FramesTest.java [32]](https://github.com/SeleniumHQ/seleniumhq.github.io/pull/1915/files#diff-c32bcc5edb4bcccf7c3694e3a6103af9a1bb389f8a7b6afee1821662f87df0baR32-R32) ```diff -driver.manage().timeouts().implicitlyWait(Duration.ofMillis(500)); +WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(10)); +wait.until(ExpectedConditions.presenceOfElementLocated(By.id("iframe1"))); ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: Using WebDriverWait instead of an implicit wait provides more precise control over wait conditions, improving test reliability and performance. | 8 | |
Enhancement |
Use a more specific assertion for checking text content___ **Replace the boolean assertion with a more specific assertion that checks for theexact text content.** [examples/java/src/test/java/dev/selenium/interactions/FramesTest.java [42]](https://github.com/SeleniumHQ/seleniumhq.github.io/pull/1915/files#diff-c32bcc5edb4bcccf7c3694e3a6103af9a1bb389f8a7b6afee1821662f87df0baR42-R42) ```diff -assertEquals(true, driver.getPageSource().contains("We Leave From Here")); +assertTrue(driver.getPageSource().contains("We Leave From Here"), "Expected text 'We Leave From Here' not found in page source"); ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: The suggestion improves the clarity of the test by using assertTrue with a descriptive message, making it easier to understand test failures. | 7 |
Combine redundant frame switching tests into a single, more efficient test method___ **Combine the two separate frame switching tests into a single test method to reduceredundancy and improve test efficiency.** [examples/java/src/test/java/dev/selenium/interactions/FramesTest.java [38-59]](https://github.com/SeleniumHQ/seleniumhq.github.io/pull/1915/files#diff-c32bcc5edb4bcccf7c3694e3a6103af9a1bb389f8a7b6afee1821662f87df0baR38-R59) ```diff -//switch To IFrame using Web Element +// Test frame switching using both WebElement and name/id WebElement iframe = driver.findElement(By.id("iframe1")); -//Switch to the frame + +// Switch to frame using WebElement driver.switchTo().frame(iframe); -assertEquals(true, driver.getPageSource().contains("We Leave From Here")); -//Now we can type text into email field -WebElement emailE= driver.findElement(By.id("email")); -emailE.sendKeys("admin@selenium.dev"); -emailE.clear(); +assertTrue(driver.getPageSource().contains("We Leave From Here"), "Expected text not found after switching by WebElement"); +WebElement emailElement = driver.findElement(By.id("email")); +emailElement.sendKeys("admin@selenium.dev"); +emailElement.clear(); driver.switchTo().defaultContent(); - -//switch To IFrame using name or id -driver.findElement(By.name("iframe1-name")); -//Switch to the frame -driver.switchTo().frame(iframe); -assertEquals(true, driver.getPageSource().contains("We Leave From Here")); -WebElement email=driver.findElement(By.id("email")); -//Now we can type text into email field -email.sendKeys("admin@selenium.dev"); -email.clear(); +// Switch to frame using name +driver.switchTo().frame("iframe1-name"); +assertTrue(driver.getPageSource().contains("We Leave From Here"), "Expected text not found after switching by name"); +emailElement = driver.findElement(By.id("email")); +emailElement.sendKeys("admin@selenium.dev"); +emailElement.clear(); driver.switchTo().defaultContent(); ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 6Why: The suggestion reduces redundancy and improves test efficiency by combining similar frame switching tests, although it may slightly reduce test granularity. | 6 |
Thank you @harsha509 :)
User description
Thanks for contributing to the Selenium site and documentation! A PR well described will help maintainers to review and merge it quickly
Before submitting your PR, please check our contributing guidelines. Avoid large PRs, and help reviewers by making them as simple and short as possible.
added content and code for frames example
Description
added content and code for frames example
Motivation and Context
added content and code for frames example as it wasn't there
Types of changes
Checklist
PR Type
enhancement, documentation
Description
FramesTest
to demonstrate iframe interactions using Selenium.Changes walkthrough ๐
FramesTest.java
Add Java test for iframe interactions using Selenium
examples/java/src/test/java/dev/selenium/interactions/FramesTest.java
FramesTest
for handling iframes.informationWithElements
to demonstrateswitching between iframes.
name, and index.
frames.en.md
Update English documentation with iframe example references
website_and_docs/content/documentation/webdriver/interactions/frames.en.md
frames.ja.md
Update Japanese documentation with iframe example references
website_and_docs/content/documentation/webdriver/interactions/frames.ja.md
iframes.
frames.pt-br.md
Update Portuguese documentation with iframe example references
website_and_docs/content/documentation/webdriver/interactions/frames.pt-br.md
iframes.
frames.zh-cn.md
Update Chinese documentation with iframe example references
website_and_docs/content/documentation/webdriver/interactions/frames.zh-cn.md
iframes.