Selenium Java: errorRecovery

Question

I have the following code to select an image using Selenium, only now I have added an errorRecovery. Is there is a better way to right the code in the catch?

    public static void selectHeader() {
    try {
        WebDriverWait wait = new WebDriverWait(driver, 20);
        wait.until(ExpectedConditions.visibilityOfElementLocated(By
                .xpath("//*[contains(@id, 'picture-und-0-select')]")));
        driver.findElement(
                By.xpath("//a[contains(@id, 'picture-und-0-select')]"))
                .click();
        driver.switchTo().frame("mediaBrowser");
        wait.until(ExpectedConditions.presenceOfElementLocated(By
                .linkText("LIBRARY")));
        driver.findElement(By.linkText("LIBRARY")).click();
        driver.findElement(By.tagName("img")).click();
        driver.findElement(By.linkText("Submit")).click();
        driver.switchTo().defaultContent();
    } catch (UnhandledAlertException alert) {
        errorRecovery error = _TestSuite.errorRecovery;
        if (error.getCount() == 0) {
            error.addCount();
            selectHeader();
        } else {
            error.resetCount();
            throw new UnhandledAlertException(alert.toString());
        }
    }
}

Answer 1

Judging by the overall structure it looks quite good, I would change a few things though with respect to formatting:

1. Your public static void selectHeader() { line seems to be off, might've been due to putting the code here, you should remove 4 blanks.

2. Take as example this line:

   wait.until(ExpectedConditions.visibilityOfElementLocated(By
          .xpath("//*[contains(@id, 'picture-und-0-select')]")));
   

   I think it would read better as:

   wait.until(ExpectedConditions.visibilityOfElementLocated(
          By.xpath("//*[contains(@id, 'picture-und-0-select')]")));
   

   Note that now every argument is on one line.

3. Similar situations a few lines below.

Then onto the exception handling:

1. First I would change UnhandledAlertException alert to UnhandledAlertException alertEx, to denote that it is about an exception.

2. errorRecovery error = _TestSuite.errorRecovery confuses me to no ends, hence these suggestions:

   • _TestSuite should not have the underscore in front of it, I am still assuming it is a class though.
   • errorRecovery appears to be a class, which really should and needs to be upper-cased to prevent any confusion I had so far.

3. Your header_afbeelding_selecteer() method call is in Dutch and not in camelCase, it should be something like headerImageSelect().

4. Lastly, your ErrorRecovery error is not an error, it is an recovery or an errorRecovery, please name your variable like that.

I do not fully get what your error handling code does though, there also seems to be logic ongoing which is not neccessary, all I see that you are really using is the method call to headerImageSelect().

Therefore I suggest to add the following to your ErrorRecovery class:

public void handleRecovery(final Runnable methodToRun, final String exceptionMessage) throws UnhandledAlertException {
    if (getCount() == 0) {
        addCount();
        methodToRun.run();
    }
    else {
        resetCount();
        throw new UnhandledAletException(exceptionMessage);
    }
}

Which can then be called as:

errorRecovery.handleRecovery(new Runnable() {
    @Override
    public void run() {
        headerImageSelect();
    }, alertEx.toString());

Or if you are using Java 8, just:

errorRecovery.handleRecovery(() -> headerImageSelect(), alertEx.toString());

or even better:

errorRecovery.handleRecovery(this::headerImageSelect, alertEx.toString());

The design still seems weird to me though, which perhaps now is even more obvious, why does your error recovery method throw an UnhandledAlertException? Indicating that your error would need recovery again!