5
\$\begingroup\$

I'm currently writing unit tests in Selenium (C#). I need to do two things in this particular test:

Check if 2 errors (with html container) are displayed and check if another (given) error is not displayed.

The test looks like this:

bool errorWindowVisible = tmethods.IsElementVisible(driver, By.XPath("//div[@class='message error']"));
if (errorWindowVisible == false)
{
  Assert.Fail("No error notification displayed.");
}

string errorTextTemp = "Error 1 text";
bool productIssue = tmethods.IsElementVisible(driver, By.XPath("//*[contains(.,'" + errorTextTemp + "')]"));
errorTextTemp = "Error 2 text";
bool productIssueB = tmethods.IsElementVisible(driver, By.XPath("//*[contains(.,'" + errorTextTemp + "')]"));
if (productIssue == false || productIssueB == false)
{
  Assert.Fail("No information about error cause.");
}

// Checking if another one is not visible
errorTextTemp = "Extra error";
bool uploadedFileIssue = tmethods.IsElementNotVisible(driver, By.XPath("//*[contains(.,'" + errorTextTemp + "')]"));    
if (uploadedFileIssue == false) // Should be set to true
{
  Assert.Fail("Extra error shown.");
}

I use two methods here:

public bool IsElementVisible(IWebDriver driver, By element)
{
    if (driver.FindElements(element).Count > 0)
    {
    if (driver.FindElement(element).Displayed)
        return true;
    else
        return false;
    }
    else
    {
    return false;
    }
}

public bool IsElementNotVisible(IWebDriver driver, By element)
{
    if (driver.FindElements(element).Count > 0)
    {
    if (driver.FindElement(element).Displayed)
        return false;
    else
        return false;
    }
    else
    {
    return true;
    }
}

So, all of that is working, but I am not sure about quality of that code. On the other hand, I don't need anything over-fancy. It just should be good, clean and understandable.

Should I keep it intact or update some parts?

\$\endgroup\$
  • 2
    \$\begingroup\$ one thing.when you have tmethods.IsElementVisible dont create tmethods.IsElementNotVisible. just negate and write !tmethods.IsElementVisible \$\endgroup\$ – M.kazem Akhgary Oct 21 '15 at 21:35
11
\$\begingroup\$

Your IsElementVisible method can be condensed to:

public bool IsElementVisible(IWebDriver driver, By element)
{
    return driver.FindElements(element).Count > 0
        && driver.FindElement(element).Displayed;
}

I guess the method IsElementNotVisible should return boolean negation of result of the IsElementVisible method:

public bool IsElementNotVisible(IWebDriver driver, By element)
{
    return !IsElementVisible(driver, element);
}

Boolean comparisons

Expressions x == true and just x are eqivalent.
Expressions x == false and just !x are also eqivalent.
For example, you don't need to write:

if (productIssue == false || productIssueB == false)

you could write instead:

if (!productIssue || !productIssueB)
\$\endgroup\$
  • \$\begingroup\$ Question about IsElementVisible - why do we count that elements and check if are displayed? Count is faster so when it fails it doesn't bother to check if element is displayed, or there is another reason for that 2 conditions? \$\endgroup\$ – Tomasz Tarnowski Feb 10 '16 at 10:50
  • \$\begingroup\$ @BlackHat Is this question for me? You should answer it, because it is your code. I've just simplified it. I don't even know what the type IWebDriver is. \$\endgroup\$ – Dmitry Feb 10 '16 at 14:41
  • \$\begingroup\$ A bit embarrassing... I totally forgot about origin of that code. Funny thing now I am not sure why have I implemented that way. Sorry for the fuss! \$\endgroup\$ – Tomasz Tarnowski Feb 10 '16 at 15:29
8
\$\begingroup\$

Test review

It is not clear from the code in your upper code block, whether you are doing three separate tests, or if that is one test method doing three assertions.

Anyway it is common to strive for that one test method has only one assertion. Having multiple tests in one test method is considered a bad test regime.

Using Assert.Fail() is also somewhat uncommon. It would be better to use Assert.IsTrue() or Assert.IsFalse() and move the condition into the actual assert.

Finally, regarding the assertions, I'm not thrilled about your texts. What information do you get from "Extra error shown" or "Error 1 text"? Let the test cases and texts be clear and informative, so that you know what actions to take if it fails at some point in time.

Style review

First of all, Dmitry has some excellent point regarding refactoring in his answer. My comments are more on the existing code in your lower block:

  • Don't mix brace styles – In some cases you have braces around the if blocks, and some places you don't. You are much better off using braces always! Even though some consider this a disease, braceophilia, you will save yourself time and trouble always including the braces

  • Clean up indentation of your code – Your code is very confusing when looking at the indentation. This is both a reason for you to include braces (which would clearly indicate blocks), and it will (with a decent editor) help you get the indentation correct.

    As it stands, it is confusing which level of the if statement the Count and/or Displayed statements are at. They should be at distinct indentation levels.

  • Avoid negated functions – There is a reason why the ! operator exists, and duplicating your logic like you've done is a code smell. Keep one of them, and make that one correct and efficient, and use the ! operator to get the opposite version. (Or in some rare cases include the negated version, but let it call the ordinary version (as suggested by Dmitry))

  • Don't use if blocks to return the condition – This is already covered by Dmitry, but instead of doing if (someCondition) { return true; } else { return false; }, simplify it to return someCondition;

\$\endgroup\$
  • 1
    \$\begingroup\$ braceophilia got me laughing ;-) \$\endgroup\$ – Heslacher Oct 22 '15 at 7:28
  • \$\begingroup\$ Thanks for all the tips, I've removed all issues you mentioned. Also small comment: I don't use error slogans like "Error 1 text", but I had to censor them due to my company restrictions :) Thanks again! \$\endgroup\$ – Tomasz Tarnowski Oct 22 '15 at 11:31

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.