4
\$\begingroup\$

I'm testing an application which is not exactly an e-commerce application but which behaves enough like one that you can think of it as an e-commerce application for the purposes of understanding this code :)

This code relies heavily on the Selenium Webdriver library.

The code comes from the Product page, where you have searched and found a list of products. This is a utility method to get the list of products being displayed so I can verify correctness along any axis. This method only gets those products that are visible, which may differ from the total list of products if filtering is involved.

I maintain a list of WebElements representing the container for the product display, one for each product on the page, which is initialized in initLists() (along with some other non-important lists) so that we don't have stale references. That's what's stored in allProducts.

ProductView is an inner class that encapsulates a specific Product record in the list; interactions with a given product (such as clicking any of the links for more information) are delegated to the ProductView for that Product.

public ArrayList<Product> getAllVisibleProducts() {
    initLists(); //Reinitialize lists

    Utils.WaitForAjax(driver);
    ArrayList<Product> retval = new ArrayList<Product>();
    for (WebElement rProduct : allProducts) {
        try {
            if (rProduct.isDisplayed()) retval.add(new ProductView(rProduct).getProduct());
        } catch (StaleElementReferenceException e) {
            continue; //We just ignore that element
        }
    }
    return retval;
}

I'm getting hangs of sometimes multiple minutes where the test appears to do nothing on the product page. I'm currently still trying to reproduce the super-long lag, but at least part of my problem is that this method takes quite a long time to return. I put log statements at various portions of the app, and between starting the loop and returning, it was something like 44 seconds for 8 products. Am I doing too much web scraping here?

(ProductView stores references to the elements that are interactable for the given product and attempts to build an in-memory representation of that product by scraping the page using Webdriver. The code to build the product is the vast majority of the constructor and clocks in about 70-odd lines, so I omitted it).

Ideally I think I need to hear from someone who has used WebDriver before, since this probably boils down to WebDriver best practices.

\$\endgroup\$
  • \$\begingroup\$ I don't have quite enough rep here to make any more specific tags. Webdriver and functional-testing seem appropriate? \$\endgroup\$ – Yamikuronue Sep 18 '14 at 14:39
  • \$\begingroup\$ We are missing a bit of the bigger picture here. Where is allProducts coming from ? What does initLists() do ? What is isDisplayed, a simple accessor or not? BTW Welcome to Code Review! \$\endgroup\$ – Marc-Andre Sep 18 '14 at 14:42
  • \$\begingroup\$ @Marc-Andre isDisplayed is a library method from WebDriver, I didn't write it. initLists() and allProducts I explained above, let me separate that out so it's easier to find and add more clarifying details. ETA: Updated \$\endgroup\$ – Yamikuronue Sep 18 '14 at 14:44
  • \$\begingroup\$ Well I blame myself since I've made a quick read of your question. I'll add a bit of formatting for the quick reader. \$\endgroup\$ – Marc-Andre Sep 18 '14 at 14:47
1
\$\begingroup\$

You claim "ProductView stores references to the elements that are interactable for the given product and attempts to build an in-memory representation of that product by scraping the page using Webdriver", could the lag simply be due to the time it takes to scrape the page then?

Is this web scraping taking place over the public Internet, or can you set up a simple caching mechanism on top of your application to reduce load times? An internal web server just to facilitate testing should work too. Ideally, you should have a stub version of ProductView that you can test without having to perform the web scraping.

The one final minor optimization I can think of (a really minor point, if I haven't emphasized enough) is to make initialize retval to the size of allProducts first, so that there is definitely no need to resize it later.

Otherwise, stick with @Marc-Andre's answer, and look towards tackling the web scraping mechanism.

\$\endgroup\$
3
\$\begingroup\$

Bunch of small things not related to your performance problem :

  • Utils.WaitForAjax should be Utils.waitForAjax. Method in Java start with a lower letter.
  • Don't return ArrayList, it's an implementation. You should use the interface List. The day you want to change to an LinkedList, it will be easier.
  • I recommend to always use {} for if, even if you only have just one line. But that is up to you.
  • I like to name ignoredException an exception that I'm ignoring. Make it more clear.

By looking at your code, I can only guess that your performance bottleneck probably is new ProductView(rProduct) or rProduct.isDisplayed(). If in ProductView your scrapping the page, it could take a lot of time depending on the driver and the method you're doing it.

But with the code your showing us right now, there is nothing particularly wrong.

\$\endgroup\$
  • \$\begingroup\$ Heh, my fellow developers at my company asked me to return ArrayList over List because they don't like to guess what kind of List I used. \$\endgroup\$ – Yamikuronue Sep 18 '14 at 15:02
  • \$\begingroup\$ @Yamikuronue Well I understand why you're doing it, but I would argue with your fellow developers why do they need to care which one you use. \$\endgroup\$ – Marc-Andre Sep 18 '14 at 15:07
  • 1
    \$\begingroup\$ @Yamikuronue maybe you can have a bit of fun with them by describing product names in full in real life from now on... \$\endgroup\$ – h.j.k. Sep 19 '14 at 0:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.