-1
\$\begingroup\$

Lately I've been getting a lot of critique in my code reviews at work, where typical comments have been that my variable names are unclear/confusing, flow in method could be simplified, unnecessary checks (e.g. null), not robust enough and a lot of other things. So I just wanted your opinion on the following test helper method that I have written as a part of an automated UI test:

public void AssertTooltipFieldMatchesExpectedText(int row, string expectedText)
{
    WpfText tooltipFieldTextControl;
    var tooltipRows = Window.FindDataGrid("TooltipElementsDataGrid").GetAllRows();

    if (tooltipRows.Count == 0)
        Assert.Fail("No tooltip rows found");

    if (tooltipRows.ElementAtOrDefault(row) != null)
    {
        tooltipRows.ElementAt(row).GetDataGridCell("Field")
            .TryFindText("", out tooltipFieldTextControl);

        Assert.IsNotNull(tooltipFieldTextControl, string.Format("Could not find text control for tooltip at the specified row: {0}", row));
        Assert.AreEqual(expectedText, tooltipFieldTextControl.DisplayText);
    }
    else
        Assert.Fail(string.Format("No tooltip found at row: {0}", row));
    }
}

Ok, so all calls to other methods are some that exist already, so I haven't written any of those, but instead trying to make use of them.

tooltipRows will always contain a list. It's empty if no rows were found.

tooltipFieldTextControl will have null assigned if no text control was found in the call to TryFindText.

Any tips on how I could improve this method? I know a lot of suggestions could be subjective, but I would still appreciate them. If you think some of the code can be made more concise while still more readable, or vice versa, let me know.

I thought about wrapping some if it in a try/catch instead, but I found that if/else is better as I expect null values in the case where it couldn't find text etc.

\$\endgroup\$
0
2
\$\begingroup\$

I will not comment on whether that approach for testing is good or not but based on the code in that method, here is my feedback:


Check for null if a null can be returned. If your reviewer says you are checking for too many nulls, just indicate that the possibility is there so I have to check for it or it will cause an exception. So you should check for nulls in the following:

// Change this 
var tooltipRows = Window.FindDataGrid("TooltipElementsDataGrid").GetAllRows();

// to this
var grid = Window.FindDataGrid("TooltipElementsDataGrid");
if (grid == null)
{
    // Not found so either fail the test or return right away
    // There is no need to do anything else
    // Early returns make this really clear
}

Here you can also return right away:

if (tooltipRows.Count == 0)
{
    Assert.Fail("No tooltip rows found");
    return;
}

Early returns are great because it lets the reader know that if this case has been met, the rest of the code does not matter. More time is spent reading code than writing code, so make it easier for reading.


Think about your code and try not to do the same thing over and over or even a second time if it is not needed. For example, in the code below you look for an element and if the element is there, then you look for it again:

// check if element is there at row
if (tooltipRows.ElementAtOrDefault(row) != null)
{
    // Now get the element at row
    tooltipRows.ElementAt(row).GetDataGridCell("Field")
        .TryFindText("", out tooltipFieldTextControl);
}

That can be re-written like this:

var element = tooltipRows.ElementAtOrDefault(row);
if (element == null)
{
    Assert.Fail(string.Format("No tooltip found at row: {0}", row));
    // Again you can leave here
}
else
{
    // use the element. No need to look for it again
}

Keep variables as close as possible to where you use them. For example the tooltipFieldTextControl variable is only used inside the if block. Therefore, move it there.


Try and write optimistic if conditions, if you can. For example the code below is much easier to read if we flip it:

if (tooltipRows.ElementAtOrDefault(row) != null)
{
}
else
{
}

To this:

if (tooltipRows.ElementAtOrDefault(row) == null)
{
}
else
{
}

We human beings do not talk or think like that-well most of us dont. We do not say:

All the people who are not 18 and over, do not go to section B.

Much easier to understand if the above was said:

All the people who are under 18, go to section A. Everyone else go to section B.

Even with plain English, you will need to pause and think about the first sentence with all the nots. But the second one is much easier because it is more natural.

There are times where a pessimistic condition will be needed. But most of the time, if you think about it, you can flip it to more a natural sounding condition.


Do not design with code. I always tell my team members not to design and think using code but think about it from a non programmer's point of view. Once you have the solution, then convert it into code. But do not write code and as you go along you try to come up with a solution. You need a solution and then as a programmer you implement that solution using programming. But if you were to give the solution to someone, they should be able to do it with a pencil and a paper without programming. Obviously it will take longer to do it manually, but you get the point. For example, in your method you need to do this:

  1. Find the text at a give row in TooltipElementsDataGrid and ensure it equals expectedText
  2. If it equals, pass the test
  3. If not equals, fail the test

That is the task, and now you need an algorithm, set of tasks to achieve that. So what is the most logical way to do that?

  1. Look for the row and if not found, fail the test. There is nothing else to be done.
  2. If the row is found, check the text. If it does not equal the expected text, fail the test. Nothing else needs to be done.
  3. If the above cases do not happen, then it is a pass.

Here is the full code:

public void AssertTooltipFieldMatchesExpectedText(int row, string expectedText)
{
   var grid = Window.FindDataGrid("TooltipElementsDataGrid");
   if (grid == null)
   {
      Assert.Fail("Grid not found");
      return;
   }

   var tooltipRows = grid.GetAllRows();

   if (!grid.Any()) // You do not need count, you just need to know if anything is there or not
   {
      Assert.Fail("No tooltip rows found");
      return;
   }

   var element = tooltipRows.ElementAtOrDefault(row);
   if (element == null)
   {
      Assert.Fail(string.Format("No tooltip found at row: {0}", row));
      return;
   }

   var cell = element.GetDataGridCell("Field");
   if (cell == null)
   {
      Assert.Fail("Field not found");
      return;
   }

   WpfText tooltipFieldTextControl;
   cell.TryFindText("", out tooltipFieldTextControl);
   Assert.IsNotNull(tooltipFieldTextControl, string.Format("Could not find text control for tooltip at the specified row: {0}", row));
   Assert.AreEqual(expectedText, tooltipFieldTextControl.DisplayText);
}

Lastly, if your reviewers are telling you: Your variables are not clear. You write too many null checks. Ask them, nicely, to please elaborate. Simply saying that is not good enough.

EDIT

Remember what I said: Check for null if a null can be returned. If this is an automated test and your reviewer says why are you checking if the item with TooltipElementsDataGrid will be null? You can come into agreement on whether to check for null or not. The argument for checking for null is what if one of the developers change the name to something else. The argument for not checking for null (your reviewers may use) is "No, that will rarely happen because we developers have agreed not to ever do that. If a developers does do it, we are ok to get a nasty exception."

In a comment to this answer, you asked:

This style of testing is used throughout the application that I'm working on! So it would be interesting to know if you have any suggestions for how it can be improved.

A good test should be very clear when it fails. In this case the name of your test is AssertTooltipFieldMatchesExpectedText. If that fails you need to figure out what exactly failed in that test. In other words, you have multiple things you are testing and this is why you have multiple asserts. A good test should have one assert. Therefore, that should be multiple tests. Now your reviewer or yourself may say but that is a lot of code. And the argument to that is: Tests should be simple and easy to write. The code should be simple too. There is a lot of code because a lot of things need to be tested. Simple.

Next problem is that test name does not tell me which item's tooltip has issues. It also does not mention which expectedText was not found.

If the tooltip for Salary grid is to show Tip1, Tip2 and Tip3, you should have 3 tests:

`EnsureSalaryGridHasTip1()`
`EnsureSalaryGridHasTip2()`
`EnsureSalaryGridHasTip3()`

If you care about specific rows then it can be:

`EnsureSalaryGridHasTip1AtRow1()`

Also, the name of the tests should be long and clear that it even makes sense to a non-programmer.

This is a very broad area so it is hard to explain it in details in such a short answer but I hope that helps. Just ask your reviewers and they will guide you.

\$\endgroup\$
5
  • \$\begingroup\$ Thanks! That helped. Some really good advice in your answer.You stated that you didn't want to comment on the testing approach, would you mind commenting on it any way? This style of testing is used throughout the application that I'm working on! So it would be interesting to know if you have any suggestions for how it can be improved \$\endgroup\$
    – Force444
    Mar 11 '17 at 18:24
  • \$\begingroup\$ Note also that C#7 allows the out param to be out var name to declare inline into the containing scope. \$\endgroup\$
    – CAD97
    Mar 12 '17 at 16:05
  • \$\begingroup\$ a return after Assert.Fail does not make any sense ;-) \$\endgroup\$
    – t3chb0t
    Mar 14 '17 at 7:24
  • \$\begingroup\$ @t3chb0t yes you are correct. I will edit my answer later today. Good catch. Although in this case the return makes no sense but my suggestions were in general. Nonetheless good catch. \$\endgroup\$ Mar 14 '17 at 11:26
  • \$\begingroup\$ @cad97 please take note of the last comment by t3chb0t \$\endgroup\$ Mar 14 '17 at 11:27

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.