For new work: Don't use ToString for unit tests

[dodexahedron]

We've got a lot of unit tests that use calls to ToString to perform checks that are either also performed on the actual values as well (making them redundant) or, in some instances, instead of checks on values, both of which are not Good Things.

Either way, please please please don't do this for tests of anything other than ToString itself. It represents coupling that is very fragile and which can break hundreds of tests from an unrelated change, when those tests actually should pass based on what they're actually testing. At minimum, it makes it more difficult to debug things when a test fails and it doesn't actually have anything to do with the test that failed.

It also represents coupling across unrelated types, in the context of testing. This can be seen in the PR that prepared for removal of Rect. Changing ToString on Rect to work like System.Drawing.Rectangle broke tests all over the place, because of this, which had to be manually updated. For some of those, I substituted a call to ToString (via interpolation) on the rectangle for the expected value. That's a little better, but still an invalid test. It was just not high on my list to fully fix at that time.

Tests including asserts on output of ToString are tests of ToString - not the value you care about and that the test claims to be for - even if the value you care about would be in that string, in the code as it exists when you write that. That assumption isn't relevant to a unit test and is fragile in multiple ways.

It's also entirely possible that some of those tests could fail for certain values, depending on the culture of the process executing the tests. What if a numeric value gets a comma in one culture but a period in another, for certain values? Not only do we not typically test ranges that thoroughly, but such tests can fail just for that reason, when the actual underlying values are perfectly correct and things are behaving properly. Another example: What if the culture doesn't use Arabic numerals for numbers? Test failed.

Where I've seen this the most, recently, has been calling ToString on a View type and checking that result against a string constant.

Those should only be checking the rectangles and other properties involved in the actual test. A change in ToString shouldn't be able to break those tests, because ToString should have its own test, on each type, which validates the behavior of its own ToString implementation, while further leaving any dependencies to do their own validation, as well.

For example, a change to ToString on Attribute or Adornment, right now, could potentially break a ton of other unrelated tests because of this. If those types test their own ToStrong methods, any types that use the output of those ToString methods to form their own ToString output are supposed to blindly trust that output. Failures aren't supposed to cascade. Therefore, ToString for those types would need to have the expected values of their tests written not as constants but as the composition of that type's output with the output of the type they depend on interpolated into it. In other words, View.ToString isn't testing Rectangle.ToString, so the part of it that is from Rectangle should be interpolated in the expected value.

And again, that's ONLY in a test of ToString. Other test methods that don't directly have something to do with ToString itself shouldn't even ever call ToString. Unit tests are supposed to be independent.

Besides, it's a maintenance nightmare to keep that all properly up to date with every change that might affect it in every test method that does it.

I'm going to convert this issue into a discussion, but am initially filing it as an issue for visibility.

[BDisp]

I absolutely agree with not using the ToString method for unit tests because if a type is changed it forces you to rectify all tests manually. If we use the type itself when renamed, it is much easier to change all occurrences using the IDE's own mechanism. But I also recommend that all occurrences that refer to the changed type be changed in comments, documents, etc.

[dodexahedron]

Yep. Agreed on all of those points. And conditionally-compiled code (#if SOME_SYMBOL) should also get the same treatment.

That's a cool one ReSharper is capable of doing for you. There's a checkbox in the rename dialog that comes up when you tell it to fix all occurrences which says something like "Apply changes in inactive conditional code" or whatever. It'll do exactly what that sounds like. It will also do it in comments and strings, for a pretty wide range of situations. Of course that's for names, not values, which is why I had to do a lot of the minor updates I made to some of those tests manually, as well, in the stuff that just got merged.

There are other goodies in the ReSharper global rename refactoring workflow, too, like also changing names of code elements directly related to the thing being renamed. For example, when I did the Rect -> Rectangle change, I used ctrl-r-r for the ReSharper rename refactoring, and it offered to change things like property names and method parameter names ending in Rect to ending in Rectangle.

And it lets you select which of those changes you actually want to let it do, too, and you can even click on the entries in the tree to go to that point in the code to inspect it yourself, if you want. I love that flexibility and thoroughness.