Checking if text is NOT displayed

Question

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?

• M.kazem Akhgary: one thing.when you have tmethods.IsElementVisible dont create tmethods.IsElementNotVisible. just negate and write !tmethods.IsElementVisible

Answer 1

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)

• Tomek: 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?
• Dmitry: @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.
• Tomek: 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!

Answer 2

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;

• Heslacher: braceophilia got me laughing ;-)
• Tomek: 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!