Do not write code that surprises its users

The following simple page class and tests are our starting point:

public class HomePage {

   //locators, URL, driver variables

   //constructor

   public void open() {
      driver.get(URL);

      Assert.assertTrue(driver.getCurrentUrl().contains(URL));
   }	

   public void doRandomSearchForMakeAndModel(String postalCode) {
      String make = getRandomMake();
      selectMake(make);

      String model = getRandomModel();
      selectModel(model);

      typePostalCode(postalCode);

      executeSearch();
   }

   public void searchFor(String make, String model, String postalCode) {
      selectMake(make);
      selectModel(model);

      typePostalCode(postalCode);

      executeSearch();
   }

   public void selectMake(String make) {
   }

   public void selectModel(String model) {
   }

   public String getRandomMake() {
   }

   public String getRandomModel(){
   }

}

@Test
public void searchReturnsResultsTest() {

   HomePage homePage = new HomePage(driver);

   homePage.open();	

   homePage.doRandomSearchForMakeAndModel(POSTAL_CODE);

   //the test continues

}

The test creates a home page object.

It uses it to open the home page and execute a search using a random make, random model and a specific postal code.

How many issues can you find in this code?

---

1. Using an assertion in a page method

The first issue is using an assertion in the open() method:

public void open() {
   driver.get(URL);

   Assert.assertTrue(driver.getCurrentUrl().contains(URL));
}

Assertions should be used only in the test methods.

Who says that, Alex, who?

The Selenium team.

Here.

From this page,

Page objects themselves should never make verifications or assertions. This is part of your test and should always be within the test’s code, never in an page object. The page object will contain the representation of the page, and the services the page provides via methods but no code related to what is being tested should be within the page object.

There is one, single, verification which can, and should, be within the page object and that is to verify that the page, and possibly critical elements on the page, were loaded correctly. This verification should be done while instantiating the page object.

So, if you need an assertion about the page being loaded, either do it in the constructor (which does not work here), or do it in the test method. And do not use an Assertion but an if Statement for it.

How do we fix this?

Add the isDisplayed() method in the HomePage class as follows and have an assertion for it in the test:

public class HomePage {

   //locators, URL, driver, constructor

   public void open() {
      driver.get(URL);
   }

   public boolean isDisplayed() {
      return driver.getCurrentUrl().contains(URL);
   }

---

2. Bad method name - doRandomSearchForMakeAndModel(String postalCode)

The method name is just bad.

The method name implies that we need to do a search which is just a complicated way of saying that we need to search.

Also the name says that the search is random which is incorrect.

The make is random, the model is random, the search is always the same.

Considering all these, how about changing the name to searchForRandomMakeAndModel(String postalCode).

That’s more clear but also a bit too looooooooong?

---

3. Inconsistent code

Since the method searches for a random make and a random model, why doesn’t it also use a random postal code?

Maybe we should change it to

public void searchForRandomMakeAndModelAndPostalCode() {
   String make = getRandomMake();
   selectMake(make);

   String model = getRandomModel();
   selectModel(model);

   String postalCode = getRandomPostalCode();
   typePostalCode(postalCode);

   executeSearch();
}

The method uses only random data now.

But it comes with no parameters and a very long name.

The test also looks better and worse:

@Test
public void searchReturnsResultsTest() {

   HomePage homePage = new HomePage(driver);

   homePage.open();	

   Assert.assertTrue(homePage.isDisplayed());

   homePage.searchForRandomMakeAndModelAndPostalCode();

   //the test continues

}

No more parameters are used for the search method.

The name of the search method is too long.

The lack of parameters is just …. surprising.

---

4. Duplicated code

The following 2 methods are just duplicated:

public void doRandomSearchForMakeAndModel(String postalCode) {
   String make = getRandomMake();
   selectMake(make);

   String model = getRandomModel();
   selectModel(model);

   typePostalCode(postalCode);

   executeSearch();
}

public void searchFor(String make, String model, String postalCode) {
   selectMake(make);
   selectModel(model);

   typePostalCode(postalCode);

   executeSearch();
}

They can be re-written in a better way:

public void doRandomSearchForMakeAndModel(String postalCode) {
   String make = getRandomMake();
   String model = getRandomModel();
   
   searchFor(make, model, postalCode);
}

public void searchFor(String make, String model, String postalCode) {
   selectMake(make);
   selectModel(model);

   typePostalCode(postalCode);

   executeSearch();
}

This code is better as it has no duplication.

But we still have an issue to cover.

---

5. Mixing up phases

Assuming that the test code looks like this:

@Test
public void searchReturnsResultsTest() {

   HomePage homePage = new HomePage(driver);

   homePage.open();	

   Assert.assertTrue(homePage.isDisplayed());

   homePage.searchForRandomMakeAndModelAndPostalCode();

   //the test continues

}

The fact that the make, model and postal code are not visible is rather surprising.

In general, a unit test has 3 phases:

• setup

• exercise

• verify

The search data (make, model, postal code) is supposed to be generated in the setup phase.

The search is supposed to be executed in the exercise phase.

In our case, the setup and the exercise phase are mixed as the search method does both the data generation and the search execution.

---

How should the page class look like then?

It should look like this:

public class HomePage {

  //locators, URL, driver, constructor

  public void open() {
     driver.get(URL);
  }

  public boolean isDisplayed() {
     return driver.getCurrentUrl().contains(URL);
  }

  public void searchBy(String make, String model, String postalCode) {
     selectMake(make);
     selectModel(model);
 
     typePostalCode(postalCode);

     executeSearch();
  }

  public void selectMake(String make) {
  }

  public void selectModel(String model) {
  }

  public String getRandomMake() {
  }

  public String getRandomModel(){
  }

  public String getRandomPostalCode() {
  }

}

The test method should be like this:

@Test
public void searchReturnsResultsTest() {

   HomePage homePage = new HomePage(driver);
   homePage.open();	

   Assert.assertTrue(homePage.isDisplayed());

   String make = homePage.getRandomMake();
   String model = homePage.getRandomModel();
   String postalCode = homePage.getRandomPostalCode();

   homePage.searchBy(make, model, postalCode);

   //the test continues

}

---

In summary, try to write code that will not surprise its users.

The code should always be easy to understand, easy to change and simple.

It should not be clever, smart or cute.