Fill out webform with Selenium

Question

I am automating a webform on a webpage. I am still learning Selenium on the road. I know a few bad/good practices but a code review is always a good thing to do. What should I improve on in my code?

Quick explanation of the code:

1. I read a JSON file with customers out.
2. Initializing some variables on the run that are needed for the form.
3. For each customer get product information (data).
4. Filling the form with data using Selenium (actions).
5. After that I update a field in my database.

The code:

public void fillOutForm() throws Exception {
    String path = "D:\\Users\\Administrator\\eclipse-workspace\\AdAutomation\\data\\json\\users\\";
    String filename = username;
    String extension = ".json";

    setPath(path, filename, extension);
    String file = getPath();

    json.Customer customers = JSONReader.getCustomers(file);

    int products = customers.getProducts().size();
    int duration_in_hours = 2;
    int interval_in_minutes = 5;
    setInterval(duration_in_hours, interval_in_minutes, products);

    int turns = db.getTurns(customers.getId());

    if(customers != null) {
        for(json.Product data : customers.getProducts()) {
            waitForLoad(driver);
            // ***********************************************************************************************
            driver.navigate().to(URL + "/syi/" + data.getGroupId() + "/" + data.getRubricId() + "/plaatsAdvertentie.html");
            Thread.sleep(150);
            // ***********************************************************************************************  
            WebElement ui_upload = wait.until(ExpectedConditions.presenceOfElementLocated(By.className("content")));    
            ui_upload.click();
            // ***********************************************************************************************  
            String images = data.getImages();

            try{
                x.controlFocus("Bestand uploaden", "", "Edit1"); //File Upload
                x.ControlSetText("Bestand uploaden", "", "Edit1", images);
                x.controlClick("Bestand uploaden", "", "Button1");
            } catch(Exception e) {
                System.out.println(e.getMessage());
            }

            WebElement ui_wait_for_upload = wait.until(ExpectedConditions.presenceOfElementLocated(By.id("reject-blik")));
            ui_wait_for_upload.click();
            // ***********************************************************************************************
            WebElement ui_title = wait.until(ExpectedConditions.presenceOfElementLocated(By.id("title")));
            ui_title.clear();
            ui_title.sendKeys(data.getTitle().toString() + " #" + turns);
            // ***********************************************************************************************
            Thread.sleep(150);
            WebElement ui_condition = wait.until(ExpectedConditions.presenceOfElementLocated(By.xpath("//*/div/select[@name='attribute[condition]']")));
            Select select = new Select(ui_condition);
            select.selectByValue(data.getConditionId().toString());             
            Thread.sleep(150);
            // ***********************************************************************************************
            WebElement ui_zip = wait.until(ExpectedConditions.presenceOfElementLocated(By.name("contactInformation.postCode")));
            ui_zip.clear();
            ui_zip.sendKeys(data.getZip());
            // ***********************************************************************************************
            String myString = data.getDescription();
            byte[] utf8Bytes = myString.getBytes("UTF8");
            String description = new String(utf8Bytes,"UTF8");
            description = description.replace("\r\n", "");

            String id = "<b>#" + turns + "</b><br>";
            String nr = "<br><b>Artikelnummer: " + data.getNumber() + "</b>.<br><br>";
            String message = "<b>Let op: Indien u vragen heeft, onthoudt u dan het artikelnr " + data.getNumber() + ".</b><b?";

            Thread.sleep(150);
            JavascriptExecutor js = (JavascriptExecutor) driver;
            js.executeScript("tinyMCE.activeEditor.setContent('" + id + nr + message + description + "<br>" + id + "')");
            Thread.sleep(150);
            // ***********************************************************************************************
            WebElement ui_price = wait.until(ExpectedConditions.presenceOfElementLocated(By.name("price.value")));
            ui_price.clear();
            ui_price.sendKeys(data.getPrice().toString());
            // ***********************************************************************************************
            WebElement ui_confirm = wait.until(ExpectedConditions.presenceOfElementLocated(By.id("syi-place-ad-button")));
            ui_confirm.click();
            // ***********************************************************************************************
            wait.until(ExpectedConditions.presenceOfElementLocated(By.id("msg-success-flagging")));
        }
    }
    db.updateTurns(customers.getId());
}

Any advice is welcome.

Answer 1

Here are some things I would improve:

• I think the method is overly loaded with code and is not readable. Consider applying Extract Method refactoring method and adding clarifying comments explaining what different parts of code do - it is not immediately clear what is happening in the code.

• since you are interacting with multiple elements on a page, it might make sense to build a Page Object abstraction around the page and logic behind filling the form out.

• make sure you definitely need the AutoIT part for the file upload - I've personally had to automated file upload logic via selenium a lot of times and I've never encountered the need to make use of extra automation tools.

  The idea is to locate the input element with type="file" and send the absolute path to the desired file to upload via sendKeys(). Sometimes the file upload input is hidden - in these cases, you can make it visible and then send the file upload path to it.

• make sure the naming is consistent. You are using both camelCase and lower_case_with_underscore variable and method naming styles.

• I don't really think that comments like // *****...***** make the code more readable. Consider putting newlines between for visual separation between blocks of code. These parts would probably naturally go away if you would extract methods and/or use Page Object pattern.

• recheck if you definitely need the thread sleep delays - you are using Explicit Waits and it should be enough to tackle timing issues.

Answer 2

Based on the comments I came to a final approach of fine-tuning the code. Thank you all for your great advise, it was all really handy and usefull!

The following is done to improve the code:

1. More clarification in naming variables, methods etc.
2. Removed some unnecessary code.
3. Removed some Thread.sleep() where possible because it slows Selenium down and it can throw errors (test this out beforehand)
4. Used the extract method more often and used for naming style only camelcase instead of both camelcase and underscore.
5. Last but not least, I added comments for each part (you can say that this is also part of clarification)

See the results of the edited code below (it is much better):

public void fillOutForm() throws Exception {
    String absolutePath = "D:\\Users\\Administrator\\eclipse-workspace\\AdAutomation\\data\\json\\users\\";
    String file = username+".json";
    String path = getPath(absolutePath, file);

    json.Customer customers = JSONReader.getCustomers(path);

    int articles = customers.getArticles().size();
    setInterval(2, 5, articles);

    int turns = db.getTurns(customers.getId());

    System.out.println("Interval is: "+getInterval());

    for(json.Article article : customers.getArticles()) {
        waitForLoad(driver);

        String request = URL + "/syi/" + article.getGroupId() + "/" + article.getRubricId() + "/plaatsAdvertentie.html";
        driver.navigate().to(request);

        Thread.sleep(150);

        by = By.className("content");
        element = waitForPresenceOfElement(by);
        element.click();

        /* Upload images section */

        String images = article.getImages();

        x.controlFocus("Bestand uploaden", "", "Edit1"); //File Upload
        x.ControlSetText("Bestand uploaden", "", "Edit1", images);
        x.controlClick("Bestand uploaden", "", "Button1");

        String[] imageArray = images.split("\\\" ");
        int totalAmountOfImages = imageArray.length;

        Boolean loadingCompleted = false;

        while(!loadingCompleted) {
            int amountOfImagesLoaded = driver.findElements(By.className("done")).size();
            System.out.println("Images loaded: " + amountOfImagesLoaded);
            loadingCompleted = amountOfImagesLoaded == totalAmountOfImages;
            if(loadingCompleted) break;
        }

        by = By.id("reject-blik");
        element = waitForPresenceOfElement(by);
        element.click();

        /* *--------------------* */

        by = By.id("title");
        element = waitForPresenceOfElement(by);
        String title = article.getTitle().toString() + " #" + turns;
        performAction(element, title);

        Thread.sleep(150);

        by = By.xpath("//*/div/select[@name='attribute[condition]']");
        element = waitForPresenceOfElement(by);
        Select select = new Select(element);
        String condition = article.getConditionId().toString();
        select.selectByValue(condition);        

        Thread.sleep(150);

        by = By.name("contactInformation.postCode");
        element = waitForPresenceOfElement(by);
        String zip = article.getZip();
        performAction(element, zip);

        Thread.sleep(150);

        /* Description section */

        String myString = article.getDescription();
        byte[] utf8Bytes = myString.getBytes("UTF8");
        String description = new String(utf8Bytes,"UTF8");
        description = description.replace("\r\n", "");

        String id = "<b>#" + turns + "</b><br>";
        String nr = "<br><b>Artikelnummer: " + article.getNumber() + "</b>.<br><br>";
        String message = "<b>Let op: Indien u vragen heeft, onthoudt u dan het artikelnr " + article.getNumber() + ".</b><b>";

        JavascriptExecutor js = (JavascriptExecutor) driver;
        js.executeScript("tinyMCE.activeEditor.setContent('" + id + nr + message + description + "<br>" + id + "')");

        Thread.sleep(150);

        /* *--------------------* */

        by = By.name("price.value");
        element = waitForPresenceOfElement(by);
        String price = article.getPrice().toString();
        performAction(element, price);

        by = By.id("syi-place-ad-button");
        element = waitForPresenceOfElement(by);
        element.click();

        // ***********************************************************************************************
        by = By.id("msg-success-flagging");
        waitForPresenceOfElement(by);

        Thread.sleep(getInterval());
    }

    db.updateTurns(customers.getId());
}

Credits to: @alecxe and @GVi

Answer 3

Some ideas for future improvements excluded those already mentioned by (@alecxe):

1. Avoid hard-coding path variables. It tends to change.

2. These lines are not very clear.

    setPath(path, filename, extension);
    String file = getPath();
   

You could do String path = getFilePath(path, filename, extention); Note, that getPath is renamed and setPath is removed.

3. You use too many variables for a single method, it's not clear why do you need them. Is it something that needs to be global? If so, make them global - try not to use too many local vars.

4. The line where you assign products will throw null reference exception if customers == null;

5. ^, if customers can really be null, then move condition right after customers assignment.

6. I foreach, I would suggest change 'data' to 'product'.

7. Change driver.navigate()... to something more readable - don't build url there. You can consider having navigation helper and calling it fluently like:

   Navigate.driver(driver).toSomePage();
   

   You won't have to duplicate url's in every method you use.

8. Forget you know Thread.sleep() exist. If you use it, you leave your tests brittle. It can be that either you wait too much and your tests are unnecessarily long or you wait too little and your tests are flaky.

+ all other comments from alecxe.

Try to improve your coding skills, test automation is about coding after all.