Let’s have a look at the following class used in a test automation project:
public class Task_7 {
WebDriver driver;
public Task_7(WebDriver driver) {
this.driver = driver;
}
public void navigateToAmazon() {
String url = "https://www.amazon.com/";
if (driver.getCurrentUrl() != url) {
driver.get(url);
}
}
public void searchProduct(String productName) {
try {
WebElement searchBox = driver.findElement(
By.xpath("//input[@id='twotabsearchtextbox']"));
searchBox.click();
searchBox.clear();
searchBox.sendKeys(productName);
WebElement searchBtn = driver.findElement(
By.id("nav-search-submit-button"));
searchBtn.click();
WebDriverWait wait = new WebDriverWait(driver,
Duration.ofSeconds(30));
wait.until(ExpectedConditions.presenceOfElementLocated(
By.xpath("//a[@class='a-link-normal']//span")));
List<WebElement> searchResults = driver.findElements(
By.xpath("//a[@class='a-link-normal']//span"));
for (int i = 0; i < searchResults.size(); i++) {
WebElement result = searchResults.get(i);
String product = result.getText();
if (product.contains(productName)) {
// Scroll the element into view
((JavascriptExecutor) driver).executeScript(
"arguments[0].scrollIntoView(true);",
result);
// Wait until the element is clickable
wait.until(ExpectedConditions.elementToBeClickable(result));
// Click using JavaScript
((JavascriptExecutor) driver).executeScript(
"arguments[0].click();",
result);
break;
}
}
String originalWindow = driver.getWindowHandle();
Set<String> windowHandles = driver.getWindowHandles();
for (String windowHandle : windowHandles) {
if (!windowHandle.equals(originalWindow)) {
driver.switchTo().window(windowHandle);
break;
}
}
// Wait for the product page to load
wait.until(ExpectedConditions.titleContains(productName));
WebElement productTitle = driver.findElement(
By.xpath("//span[@id='productTitle']"));
String productTitleText = productTitle.getText().trim();
System.out.println(productTitleText);
Assert.assertEquals(productName, productTitleText);
try {
WebDriverWait product = new WebDriverWait(driver,
Duration.ofSeconds(10));
product.until(ExpectedConditions.presenceOfElementLocated(
By.xpath("(//div[@class='a-cardui'])[2]")));
WebElement product2 = driver.findElement(
By.xpath("(//div[@class='a-cardui'])[2]"));
if (product2.isDisplayed()) {
WebElement AddtoCart = driver.findElement(
By.xpath("(//input[@name='submit.addToCart'])[1]"));
AddtoCart.click();
}
}
catch (Exception e) {
// TODO: handle exception
e.printStackTrace();
}
try {
WebDriverWait wait3 = new WebDriverWait(driver,
Duration.ofSeconds(10));
wait3.until(ExpectedConditions.presenceOfElementLocated(
By.xpath("//input[@id='add-to-cart-button']")));
WebElement AddtoCart = driver.findElement(
By.xpath("//input[@id='add-to-cart-button']"));
AddtoCart.click();
} catch (Exception e) {
// TODO: handle exception
e.printStackTrace();
}
try {
WebDriverWait wait_1 = new WebDriverWait(driver,
Duration.ofSeconds(10));
wait_1.until(ExpectedConditions.presenceOfElementLocated(
By.xpath("//h1[@class='a-size-medium-plus']")));
WebElement AddToCart = driver.findElement(
By.xpath("//h1[@class='a-size-medium-plus']"));
String addToCartText = AddToCart.getText();
if (addToCartText.contains("Added to Cart")) {
driver.close();
driver.switchTo().window(originalWindow);
}
else {
driver.close();
driver.switchTo().window(originalWindow);
}
} catch (Exception e) {
// TODO: handle exception
e.printStackTrace();
}
} catch (Exception e) {
e.printStackTrace();
}
}
public Boolean CalculateTotalAmount() {
.................
}
}How do you like this test class?
What do you think about it?
Test automation code should be:
short (like in short classes, short methods)
simple (not complex)
easy to understand
object oriented
following common programming practices
easy to change and maintain
Sadly, this class does not do any of these.
It has a lot of issues such as
complexity
too long
hard to understand
not implementing common programming practices
naming issues
no synchronization
incorrect synchronization
many others
It is normal that code has issues.
No one can write perfect code.
What is not right is to leave these issues unaddressed.
But to address the issues, first, you have to find them.
How do you do this?
The most effective way of finding issues in the existing code is through code reviews.
The code reviews should be done by someone who has more experience than the author of the code (senior SDET, automation lead, developer).
This process has usually 3 phases:
read the code
provide feedback on how to improve it
implement the suggestions
review the implementation
repeat until there are no more issues
Let’s see what feedback could be provided for our code:
//FEEDBACK: the class name is not good; what does Task_7 mean?
public class Task_7 {
//FEEDBACK: all class fields should be private
WebDriver driver;
public Task_7(WebDriver driver) {
this.driver = driver;
}
/*
FEEDBACK:
since each test should start with a new browser,
there is no need to check if the url is correct or not
*/
public void navigateToAmazon() {
//FEEDBACK: this should be a constant declared at the class level
String url = "https://www.amazon.com/";
/*
FEEDBACK:
if statement is not useful.
but never use != or == to compare Strings.
*/
if (driver.getCurrentUrl() != url) {
driver.get(url);
}
}
/*
FEEDBACK:
- this method is very long with 100 lines of code
- this method is very complex as it uses multiple try/catch, for,
if/else statements
- its name is incorrect as the method does lots of other things
in addition to searching for a product
- it does the following:
1. search for keyword
2. find all results
3. select a result
4. click the selected result
5. switch to the new window opened after clicking the result
6. verify that the product title in the new window is the same
with the search keyword
7. add product to cart
8. verify that the cart is updated
- it should be broken down in many small methods
*/
public void searchProduct(String productName) {
//FEEDBACK: no need to use a try statement, it is ok if the code fails
try {
/*
FEEDBACK:
the locator should be a class variable so it can be reused.
synchronization should always be used before finding an element.
*/
WebElement searchBox = driver.findElement(
By.xpath("//input[@id='twotabsearchtextbox']"));
searchBox.click();
searchBox.clear();
searchBox.sendKeys(productName);
/*
FEEDBACK:
- no synchronization before finding the element
- the locator should be a class variable so it can be reused
*/
WebElement searchBtn = driver.findElement(
By.id("nav-search-submit-button"));
searchBtn.click();
WebDriverWait wait = new WebDriverWait(driver,
Duration.ofSeconds(30));
/*
FEEDBACK:
- incorrect synchronization
- the wait should be until the number of elements is > than 0
- the locator should be a class variable so it can be reused
*/
wait.until(ExpectedConditions.presenceOfElementLocated(
By.xpath("//a[@class='a-link-normal']//span")));
List<WebElement> searchResults = driver.findElements(
By.xpath("//a[@class='a-link-normal']//span"));
/*
FEEDBACK
this looping statement is too complex
it should be broken down in 2 parts:
1. select the product to be clicked
2. scroll to the product and click it
*/
for (int i = 0; i < searchResults.size(); i++) {
WebElement result = searchResults.get(i);
String product = result.getText();
if (product.contains(productName)) {
//FEEDBACK: DO NOT USE COMMENTS TO EXPLAIN WHAT THE CODE DOES
// Scroll the element into view
/*
FEEDBACK:
(JavascriptExecutor) driver can be saved in a variable
so it is not repeated again.
*/
((JavascriptExecutor) driver).executeScript(
"arguments[0].scrollIntoView(true);",
result);
//FEEDBACK: DO NOT USE COMMENTS TO EXPLAIN WHAT THE CODE DOES
// Wait until the element is clickable
wait.until(ExpectedConditions.elementToBeClickable(result));
//FEEDBACK: DO NOT USE COMMENTS TO EXPLAIN WHAT THE CODE DOES
// Click using JavaScript
((JavascriptExecutor) driver).executeScript(
"arguments[0].click();",
result);
break;
}
}
String originalWindow = driver.getWindowHandle();
Set<String> windowHandles = driver.getWindowHandles();
//FEEDBACK: this looping is too complex, a simpler way is needed
for (String windowHandle : windowHandles) {
if (!windowHandle.equals(originalWindow)) {
driver.switchTo().window(windowHandle);
break;
}
}
// Wait for the product page to load
wait.until(ExpectedConditions.titleContains(productName));
/*
FEEDBACK:
- the locator should be created as a class variable
so it can be reused
- no synchronization before finding the element
*/
WebElement productTitle = driver.findElement(
By.xpath("//span[@id='productTitle']"));
String productTitleText = productTitle.getText().trim();
//FEEDBACK: do not display information in console
System.out.println(productTitleText);
Assert.assertEquals(productName, productTitleText);
/*
FEEDBACK:
try statement is not useful
especially since there is no code to manage the exception
in catch clause
*/
try {
/*
FEEDBACK:
- too many waits are created in the same method
- product is not a good name for a wait object
*/
WebDriverWait product = new WebDriverWait(driver,
Duration.ofSeconds(10));
/*
FEEDBACK:
- the locator should be a class variable so it can be reused
- the expected condition is not specific enough
*/
product.until(ExpectedConditions.presenceOfElementLocated(
By.xpath("(//div[@class='a-cardui'])[2]")));
WebElement product2 = driver.findElement(
By.xpath("(//div[@class='a-cardui'])[2]"));
//FEEDBACK: what happens if the product is not displayed?
if (product2.isDisplayed()) {
/*
FEEDBACK:
- AddtoCart should be addToCartButton
- locator should be a class variable so it can be reused
*/
WebElement AddtoCart = driver.findElement(
By.xpath("(//input[@name='submit.addToCart'])[1]"));
AddtoCart.click();
}
} catch (Exception e) {
/*
FEEDBACK:
the catch should manage exception;
this code is ignoring exception;
ignoring the exception is a bad thing as usually
the execution should be stopped if an exception is generated
unless the exception is managed
*/
// TODO: handle exception
e.printStackTrace();
}
//FEEDBACK: try statement is not useful
try {
//FEEDBACK: too many waits are created in the same method
WebDriverWait wait3 = new WebDriverWait(driver,
Duration.ofSeconds(10));
wait3.until(ExpectedConditions.presenceOfElementLocated(
By.xpath("//input[@id='add-to-cart-button']")));
//FEEDBACK: AddtoCart is a bad name
WebElement AddtoCart = driver.findElement(
By.xpath("//input[@id='add-to-cart-button']"));
AddtoCart.click();
} catch (Exception e) {
// TODO: handle exception
e.printStackTrace();
}
//FEEDBACK: try is not useful
try {
//FEEDBACK: same as above
WebDriverWait wait_1 = new WebDriverWait(driver,
Duration.ofSeconds(10));
//FEEDBACK:same as above
wait_1.until(ExpectedConditions.presenceOfElementLocated(
By.xpath("//h1[@class='a-size-medium-plus']")));
WebElement AddToCart = driver.findElement(
By.xpath("//h1[@class='a-size-medium-plus']"));
String addToCartText = AddToCart.getText();
//FEEDBACK: same code is used both in if and else clauses
if (addToCartText.contains("Added to Cart")) {
driver.close();
driver.switchTo().window(originalWindow);
} else {
driver.close();
driver.switchTo().window(originalWindow);
}
} catch (Exception e) {
// TODO: handle exception
e.printStackTrace();
}
} catch (Exception e) {
e.printStackTrace();
}
}
}We will see in the next part the implementation of the feedback.