Test Smells: Cleaning up E2E Tests

In practical terms, knowing how not to write tests might be as important as knowing how to write them. There are some very helpful chapters on test smells in Gerard Meszaros's book about xUnit patterns — and more great stuff around the internet; however, it's always helpful to have practical examples for particular tech stacks.

We've already shown how to clean up unit tests; this time, we'll do JUnit + Selenide end-to-end tests, at the top of the pyramid. We're assuming you're familiar with Selenide, but most stuff here is valid for other stacks, too.

All our examples are available at a GitHub repo. There is the first, raw version of the code, the amended one that still has some issues, and the final version.

Before We Start: To Hide or Not To Hide

There are two approaches to writing automated tests:

1. Stuffing as much as possible into page objects and keeping the bare minimum in tests;
2. Keeping the tests fairly verbose.

Neither approach is universally better. The first approach is preferable if you have a complicated UI and a lot of code is being reused. On the other hand, if most complexity lies on the back-end side and you're planning to test it via API bypassing the UI, then keeping the page objects leaner is probably the better way.

Either way is fine, as long as your team is in general agreement about what you're doing. Here, we'll follow the first approach to show some structural problems.

The First, Lousy Version

Now, for our example. We get handed some tests — and we don’t like them:

import com.codeborne.selenide.Condition;
import com.github.javafaker.Faker;
import io.qameta.allure.Step;
import org.junit.jupiter.api.Test;

import static com.codeborne.selenide.Selenide.$;
import static com.codeborne.selenide.Selenide.open;

public class BadE2E {

    // The step has no description, we don't know what it's doing.
    // We can't be sure that the page is loaded
    @Step
    public void openAuthorizationPage() {
        open("<https://www.saucedemo.com>");
    }

    // Elements haven't been put inside variables or page objects,
    // which means we can't reuse them.
    // Data is hard-coded, while it could be accepted from the outside.
    @Step
    public void authorize() {
        $("#user-name").setValue("standard_user");
        $("#password").setValue("secret_sauce");
        $("#login-button").click();
    }

    // Selectors are based on classes and not IDs
    @Step
    public void checkUserAuthorized() {
   		$(".app_logo").shouldBe(Condition.visible);
    }

    // Unsafe check: if the page takes a while to load,
    // the check will pass, because the logo is present
    // on the previous page
    @Step
    public void checkUserNotAuthorized() {
    	$(".login_logo").shouldBe(Condition.visible);
    }

    // The test doesn't test anything
    @Test
    public void shouldAuthorizeUser() {
        openAuthorizationPage();
        authorize();
    }

    // 1. Opening the main page isn't put into a fixture
    // 2. The 'authorize()' method isn't reused
    // 3. Data is generated in a separate class, which currently is overkill
    @Test
    public void shouldNotAuthorizeUserWithInvalidPassword() {
        Faker faker = new Faker();
        TestUser user = new TestUser();

        openAuthorizationPage();

        $("#user-name").setValue(user.username);
        $("#password").setValue(faker.internet().password());
        $("#login-button").click();

        checkUserNotAuthorized();
    }

    // An instance of the Faker class is created in each test,
    // although just one would have sufficed.
    // Tests are pretty much identical; they should be parameterized.
    @Test
    public void shouldNotAuthorizeUserWithInvalidUsername() {
        Faker faker = new Faker();
        TestUser user = new TestUser();

        openAuthorizationPage();

        $("#user-name").setValue(faker.name().username());
        $("#password").setValue(user.password);
        $("#login-button").click();

        checkUserNotAuthorized();
    }

    // A single test has multiple checks. The tests depend on each other,
    // which may cause flakiness.
    // The checks are identical.
    @Test
    public void shouldNotAuthorizeUserWithEmptyAndBlankInputs() {
        openAuthorizationPage();

        $("#login-button").click();

        checkUserNotAuthorized();

        $("#user-name").setValue(" ");
        $("#password").setValue(" ");
        $("#login-button").click();

        checkUserNotAuthorized();

        $("#user-name").clear();
        $("#password").clear();
        $("#login-button").click();

        checkUserNotAuthorized();
    }
}

There are plenty of problems here; we'll start with the more general ones and then drill down to more specific stuff.

Refactoring

Readability

A lot of the stuff below will related to readability indirectly, but there are no glaring direct problems here; for instance, the names are all alright.

Still, we could add descriptions to our steps. We are already using the Allure @Step annotation to denote steps; we might as well use its functionality to provide a description for the step (i. e. @Step("Open the login page")). In addition to being a handy form of documentation, this also allows you to create a report with tests that anyone can read, without necessarily knowing Java. Here's an example:

Structure

There is a lot of unnecessary repetition in the tests. By removing it, we can improve their structure and make them shorter, thus easier to read. So yeah, the structure is also about readability.

For instance, every test requires us to open the registration page, so we might as well move this part into a fixture before each test. Also, all tests use the same URL, so it would make sense to move it to Configuration.baseUrl.

@BeforeEach
public void setUp() {
    Configuration.baseUrl = "https://www.saucedemo.com";
    openAuthorizationPage();
}

With this, opening the home page is just open("").

Another problem is that we're creating a Faker instance for every test. However, that's superfluous; we can store just one as a class field.

Also, it's a good idea to move selectors into separate variables:

SelenideElement inputUsername = $("#user-name");
SelenideElement inputPassword = $("#password");
SelenideElement buttonLogin = $("#login-button");

This way, you don't need to memorize or look up the selector when you're writing a new test. Also, code becomes easier to maintain if you need to change the selector for some reason.

The authorize() Method

Continuing the topic of reusability, we need to improve the method with which we authorize on the webpage:

@Step
public void authorize() {
    $("#user-name").setValue("standard_user");
    $("#password").setValue("secret_sauce");
    $("#login-button").click();
}

Currently, data is hard-coded, so we can only use the method for this particular login/password pair. Which is why we can't use it in the last two tests, e. g. here:

$("#user-name").setValue(faker.name().username());
$("#password").setValue(user.password);
$("#login-button").click();

So, how do we improve the method?

When authorizing, we're either using a default value or one generated by a faker. Maybe we could signal our method to which one to use? If we did that, we'd get a method like this:

@Step
public void authorize(Boolean username, Boolean password) {
    String trueUsername = "standard_user";
    String truePassword = "secret_sauce";

    if (username && password) {
        inputUsername.setValue(trueUsername);
        inputPassword.setValue(truePassword);
        buttonLogin.click();

        checkUserAuthorized();
    } else if (username) {
        inputUsername.setValue(trueUsername);
        inputPassword.setValue(faker.internet().password());
        buttonLogin.click();

        checkUserNotAuthorized();
    } else if (password) {
        inputUsername.setValue(faker.name().username());
        inputPassword.setValue(truePassword);
        buttonLogin.click();

        checkUserNotAuthorized();
    } else {
        inputUsername.setValue(faker.name().username());
        inputPassword.setValue(faker.internet().password());
        buttonLogin.click();

        checkUserNotAuthorized();
    }
}

Abort, abort! This looks even more horrible. Conditional logic is a major smell in a test or a step, they become more difficult to read.

Not only that, we haven't even achieved what we wanted: making the step more flexible. We still can't use it for the shouldNotAuthorizeUserWithEmptyAndBlankInputs test.

And there is another problem. Here's how the step looks like when used inside a test: authorize(true, false).

What's "true" and "false" here? What does this mean? We have to look inside the step to find out - so readability suffers and time to analyze failure increases.

The problem is, our step knows too much. Let's pass the parameters from the outside instead of setting their values inside the test:

public void authorize(String username, String password) {
    inputUsername.setValue(username);
    inputPassword.setValue(password);

    buttonLogin.click();
}

Now, we can use it everywhere we're trying to log in, and it looks perfectly self-evident when called in a test: authorize(username, password).

Parameterizing

The shouldNotAuthorizeUserWithInvalidPassword() test, the shouldNotAuthorizeUserWithInvalidUsername() test, and one of the cases from shouldNotAuthorizeUserWithEmptyAndBlankInputs() are pretty much identical. So, let's parameterize them:

@ParameterizedTest(name = "{0}")
@MethodSource("invalidCredentials")
@DisplayName("User can't authorize with ")
public void shouldNotAuthorizeUserWithInvalidCredentials(String username, String password) {
    authorize(username, password);

    checkUserNotAuthorized();
}

private static Stream<Arguments> invalidCredentials() {
    return Stream.of(
            Arguments.of("invalid password", trueUsername, faker.internet().password()),
            Arguments.of("invalid username", faker.name().username(), truePassword),
            Arguments.of("blank fields", " ", " ")
    );
}

Parameterization is great for obvious reasons - you write and maintain fewer tests. Also, each test gets a proper display name, which is provided with arguments. Take care, though: it is possible to overdo it with parameterizing.

In testing, the balance between avoiding repetition and readability is somewhat towards readability (see DRY vs. DAMP). Forcing many test cases into a single one with conditionals and dozens of parameters hurts readability greatly.

More parameters mean your test becomes more abstract and more removed from the particular problem you're testing for. Also, when it fails, you must ask yourself: is the problem in my code or in my test?

We've seen companies that managed to parameterize the hell out of everything and run tests with several thousand parameters. Somehow, they manage to get everything working, but all we can say is:

Splitting

The tests from before had to be merged, but the last test in our big example needs the opposite: splitting into multiple smaller ones. Here is its first version:

@Test
public void shouldNotAuthorizeUserWithEmptyAndBlankInputs() {
    openAuthorizationPage();

    $("#login-button").click();

    checkUserNotAuthorized();

    $("#user-name").setValue(" ");
    $("#password").setValue(" ");
    $("#login-button").click();

    checkUserNotAuthorized();

    $("#user-name").clear();
    $("#password").clear();
    $("#login-button").click();

    checkUserNotAuthorized();
}

Our tests should be atomic: when one thing fails, it shouldn't cause everything else to fail, so that we can see immediately where the problem is.

Multiple checks increase the chance of a flaky test - because every check can be flaky, and just one is enough to destabilize the test. Whenever you ask an external system for authorization, something might go wrong. For instance, something might snatch your input from you, and then all the next checks will fail regardless of what went on in the system under test.

To avoid that:

• We have to minimize the footprint of every test,
• And we have to make the tests independent of each other.

So, how do we split our test?

As it turns out, one of the checks in it has already been taken care of by parameterization, described in the previous section. Another check is simply redundant because doing .clear() and logging in is the same as just logging in. So, in the end, we can reduce our long test to this:

@Test
public void shouldNotAuthorizeUserWithEmptyInputs() {
    buttonLogin.click();

    checkUserNotAuthorized();
}

Removing a Class

In most previous examples, we've been adding abstractions - parameterizing, hiding stuff in methods, etc. But we've also got an abstraction that is unnecessary. Two of our methods use the TestUser class:

public class TestUser {

    Faker faker = new Faker();

    String username;
    String password;

    TestUser() {
        this.username = faker.name().username();
        this.password = faker.internet().password();
    }
}

It's pretty and all, but, as said, we're only using it twice in our code. Actually, both places are in tests that we want to parameterize, so in the end, it's just once.

Too many abstractions clutter the code and make it less readable. You might argue thatTestUser is a class we will need in the future. If that is true, we'll create it when we need it.

Overpreparing for the future is not a good idea. Having a modular and scalable structure is one thing, but adding more code because you might need it is another. Keep it simple:

Problems Talking to Webpages

Our example has some problems that are specific to UI tests for webpages.

Locating by IDs and Data Attributes

Let's take a look at the selectors in our tests:

@Step
public void checkUserAuthorized() {
	$(".app_logo").shouldBe(Condition.visible);
}

Whenever possible, we should base our selectors on IDs or data attributes, and not classes: they are much more precise, which means your tests are going to be less buggy. So a better version would be this:

@Step
public void checkUserAuthorized() {
	$("[data-test='secondary-header']").shouldBe(Condition.visible);
}

Making Checks Definitive

Let's take a look at this check:

@Step
public void checkUserNotAuthorized() {
	$(".login_logo").shouldBe(Condition.visible);
}

Sure, if we can see the login logo, it could mean that authorization has been rejected; however, it could also mean that the next page simply took too long to load.

Let's try to fix this:

@Step
public void checkUserNotAuthorized() {
    $(".login_logo").shouldBe(Condition.visible);
    $("#login_button_container").shouldBe(Condition.visible);
    inputUsername.shouldBe(Condition.visible);
    inputPassword.shouldBe(Condition.visible);
}

Great, we have one ID-based selector now. But this step is still bad. We've tried to compensate for an unreliable check by adding three more unreliable checks. Which means:

• If the test fails, we still aren't certain it's a "real" failure, so we'll have to waste time checking;
• More lines mean more stuff can go wrong;
• The test becomes unclear.

We really only need two checks:

• That the URL has changed;
• That the page at the new URL is the one we need.

Putting everything together, we can rewrite both this step and the one from the previous section:

@Step("Check the product page is opened")
public void checkUserAuthorized() {
    webdriver().shouldHave(url(Configuration.baseUrl + "/inventory.html"));
    $("[data-test='secondary-header']").shouldBe(Condition.visible);
}

@Step("Check the user wasn't redirected to the products page")
public void checkUserNotAuthorized() {
    webdriver().shouldHave(url(Configuration.baseUrl + "/"));
    $("#login_button_container").shouldBe(Condition.visible);
}

Load Times, Again

There is another place in our code where load time is an issue:

@Step
public void openAuthorizationPage() {
    // The URL of the page is already in Configuration.baseUrl
    open("");
}

We can't be sure that the page has been loaded, so an extra check is needed at the end:

@Step("Open the login page")
public void openAuthorizationPage() {
    open("");
    inputUsername.shouldBe(Condition.visible);
}

Tests Must Have Checks

Finally, we've got a test where we're just performing an action without checking for anything:

@Test
public void shouldAuthorizeUser() {
    openAuthorizationPage();
    authorize();
}

This is pointless —we're just "making sure it runs." The test doesn't do what is stated in the name - we don't know if the user has actually been authorized, so the test doesn't even check the happy path. So we have to add a checkUserAuthorized() call at the end. With all the other changes we've covered above, the new test is going to look like this:

@Test
public void shouldAuthorizeUserWithValidCredentials() {
    authorize(trueUsername, truePassword);

    checkUserAuthorized();
}

The Final Version

import com.codeborne.selenide.Condition;
import com.codeborne.selenide.Configuration;
import com.codeborne.selenide.SelenideElement;
import com.github.javafaker.Faker;
import io.qameta.allure.Step;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

import java.util.stream.Stream;

import static com.codeborne.selenide.Selenide.*;
import static com.codeborne.selenide.WebDriverConditions.url;

public class GoodE2E {

    static Faker faker = new Faker();

    SelenideElement inputUsername = $("#user-name");
    SelenideElement inputPassword = $("#password");
    SelenideElement buttonLogin = $("#login-button");

    static String trueUsername = "standard_user";
    static String truePassword = "secret_sauce";

    @BeforeEach
    public void setUp() {
        Configuration.baseUrl = "https://www.saucedemo.com";
        openAuthorizationPage();
    }

    @Step("Open the login page")
    public void openAuthorizationPage() {
        open("");
        inputUsername.shouldBe(Condition.visible);
    }

    @Step("Authorize with credentials: {0}/{1}")
    public void authorize(String username, String password) {
        inputUsername.setValue(username);
        inputPassword.setValue(password);

        buttonLogin.click();
    }

    @Step("Check the product page is opened")
    public void checkUserAuthorized() {
        webdriver().shouldHave(url(Configuration.baseUrl + "/inventory.html"));
        $("[data-test='secondary-header']").shouldBe(Condition.visible);
    }

    @Step("Check the user wasn't redirected to the products page")
    public void checkUserNotAuthorized() {
        webdriver().shouldHave(url(Configuration.baseUrl + "/"));
        $("#login_button_container").shouldBe(Condition.visible);
    }

    @Test
    public void shouldAuthorizeUserWithValidCredentials() {
        authorize(trueUsername, truePassword);

        checkUserAuthorized();
    }

    @ParameterizedTest(name = "{0}")
    @MethodSource("invalidCredentials")
    @DisplayName("User can't authorize with ")
    public void shouldNotAuthorizeUserWithInvalidCredentials(String username, String password) {
        authorize(username, password);

        checkUserNotAuthorized();
    }

    @Test
    public void shouldNotAuthorizeUserWithEmptyInputs() {
        buttonLogin.click();

        checkUserNotAuthorized();
    }

    private static Stream<Arguments> invalidCredentials() {
        return Stream.of(
                Arguments.of("invalid password", trueUsername, faker.internet().password()),
                Arguments.of("invalid username", faker.name().username(), truePassword),
                Arguments.of("blank fields", " ", " ")
        );
    }
}

Conclusion

On the surface, some of our recommendations might seem contradictory. We've been talking about avoiding unnecessary abstractions, the dangers of too much DRY, and the importance of keeping things simple — and yet most of our refactoring was about removing duplication. We've added more structure than we've removed, and our import list has grown quite a bit.

It might be tempting to say, "There is no silver bullet" — but there kind of is. Take your test isolated from the rest of the code, and see if you can read it without opening anything it calls. See how quickly you can read it. See if another person can read it. Imagine that you've just had to read 10 or 100 such tests. And do everything to reduce the work needed to figure out what is being tested.

Though, of course —there is no silver bullet, and intuition that comes with experience is paramount. So get on testing!