Note: I am not a C# programmer.
On the one hand, you've got "do only one thing in a test". On the other you have the DRY Principle. You're being asked which to violate. This depends on how badly you're violating each of them, how much benefit you get out of the violation, and why those rules exist in the first place.
Your grouped solution is not ideal, because it still repeats itself. If instead you did this...
[TearDown]
public void Cleanup()
{
Hotkeys.UnregisterAllLocals();
Hotkeys.UnregisterAllGlobals();
}
[Test]
public void IsRegisteredGlobal_InputNormalization()
{
Hotkeys.RegisterGlobal("Ctrl+Alt+P", delegate { });
Assert.IsTrue(Hotkeys.IsRegisteredGlobal("Alt+P+Ctrl"), "order independent");
Assert.IsTrue(Hotkeys.IsRegisteredGlobal("ctrl+alt+p"), "case insensitive");
Assert.IsTrue(Hotkeys.IsRegisteredGlobal("Ctrl + Alt + P"), "whitespace independent");
}
Then you're not violating DRY and you're hardly scuffing "do only one thing in a test". Its still doing one "thing" and that thing is normalizing the input of IsRegisteredGlobal.
You do just one thing per test in order to isolate them. This makes tests easier to isolate and debug. This doesn't mean one assert per test. The test above is still doing only one thing, but it is testing it in three very, very similar ways. This is ok. Previously your asserts were explained by the test name. Now they are explained by a message associated with each assert. The reason for failure remains obvious, the I in FIRST.
Furthermore, if you were to write all your tests with one assert per method and repeat the same code over and over unnecessarily, you not just violate DRY but the repeated code may start to slow things down violating the F in FIRST.
Is it possible that checking Hotkeys.IsRegisteredGlobal("Alt+P+Ctrl")
might cause Hotkeys.IsRegisteredGlobal("ctrl+alt+p")
to become true when if you reversed their order it wouldn't be? Yes. But there's always a trade off between isolation, speed and convenience. If you suspect one might interfere with another, you should isolate them. I'd say that if you wanted to check there's no coupling between then you should do that explicitly, in its own test, rather than saddling every test with that burden.
Yes, its fine to run code and do multiple asserts on it, but always remember it is a balancing act between good code and good tests. Usually the testing wins, but don't get silly about it.
I switched it to IsTrue
rather than the generic That
for compactness, clarity and
potentially better failure diagnostics. Assert.That( thing, condition )
means you don't know what you're testing for until the end. You have to read the whole line, see what the condition is, and read the whole line again in light of what you're really testing for. Assert.IsTrue
tells you up front. Assert.That
may be more readable in the complex assertions, but its less readable in the simple ones. It's ok to use them as appropriate. (Note: Perl programmer)
It can also potentially produce better failure diagnostics because you're giving NUnit more information about your intent. It's not "this matches that" but "this is true" so it can produce a more exact and crafted assert. Though NUnit might also be smart enough to see that you're testing against Is.True
and do that anyway. It doesn't hurt.
Assert.IsTrue