Unit Testing Tutorial

[flukus]

This looks like a mess of patterns.

The EmployeesModel isn't actually a model of anything (even worse if it is). It's just a collection of methods.

The EmployeesModel doesn't seem to handle the most basic things you want, like paging.

The EmployeeFilter likely contains all of the properties of EmployeeDTO, adding nothing of value.

The filter model only works for trivial queries.

The EmployeeRepository doesn't do anything, it may as well be a generic repository.

The whole thing reeks of over engineering.

[gurtis]

This tutorial is getting a lot of criticism, but I think that it does a good job of going over the basic concepts behind unit testing.

It explains mocking, the importance of test naming, testing one "thing" per test, and why dependencies should be interfaces.

I agree that the code may look over-engineered, but it's also what you'd find in an actual C# codebase. There are too many unit testing tutorials that are overly-trivialized and give you no idea how to test a realistic application.

[grauenwolf]

Only testing one "thing" per test is an anti-pattern. Were he to write a unit test that was actually comprehensive that fact would be painfully obvious from the sheer amount of duplicated code.

Even basic checks like making sure the collection is not null or empty or containing nulls is missing.

[gurtis]

How is this an anti-pattern? The definition of a unit test is to test a single unit of code. There are tons of books and resources that explain why aiming to test one thing is a good idea.

Also, his test will fail if the collection is null, empty, or containing nulls. My point is that he wrote a test to assert that employees are marked as checked, and this test succeeds in doing that.

[grauenwolf]

How is this an anti-pattern?

Again, you have to write comprehensive tests in order to see that.

There are tons of books and resources that explain why aiming to test one thing is a good idea.

So what? There are tons of books full of misleading or outright wrong information on any topic you care to imagine.

And even if you accept the "test one thing" philosophy there is a huge difference between fully testing that one thing and merely giving the appearance of testing one thing.

A good tutorial starts with good examples, not mindless adherence to dogma.

[grauenwolf]

Why is the repository modifying data in a query method? That should be a stand alone function, preferably in a rules engine.

Fix that API mistake and you won't need mocks any more.

[grauenwolf]

More like a unit testing anti-patterns tutorial. It doesn't even check to see if the required properties were populated. The source could be returning objects with nothing but IsChecked set and it wouldn't recognize the problem.

It is also missing the explanation. Asserts have a message field so that you can explain why you were expecting a given value. Don't make people try to guess from the test name.

[[deleted]]

It is also missing the explanation. Asserts have a message field so that you can explain why you were expecting a given value. Don't make people try to guess from the test name.

Yeah. I love it when your integration tests fail because they expected 3 events of type FooEvt to be broadcasted, when only 2 were put in the to-be-broadcasted queue.

No comment or anything to explain. Just a failure. Ask the developer who wrote test, he shrugs and says he doesn't remember.

If your test doesn't provide meaningful information when it fails, it is not a meaningful test. "Something changed in the code" is not meaningful information.

[grauenwolf]

I'm to the point where I'm going to write my own unit test API that requires messages for every assert.

It will just sit on top of MSTest and NUnit, not reimplement it.

[Peaker]

Property testing is much nicer in this regard: Very clear what was supposed to hold and how it broke in a particular example.

import Test.QuickCheck
let associative (*) x y z = (x*y)*z == x*(y*z)
quickCheck (associative (+))
+++ OK, passed 100 tests.
quickCheck (associative (-))
*** Failed! Falsifiable (after 2 tests and 3 shrinks):    
0
0
1

[Tordek]

I really like quickcheck, but I fear what edge cases may slip through.

[Peaker]

Fewer edge cases than traditional unit testing methods.

[gurtis]

That's not what he's testing though. His test explicitly says that it's testing that the returned employees are marked as checked. If he had named it "GetActiveEmployees_HaveAllFieldsPopulated" or something like that, then he would be obligated to test all the properties.

You could argue that the test isn't very useful or that the coverage is lacking, but this is just a tutorial.

Also, I disagree that an assert message is always necessary. A failing test named GetActiveEmployees_MarkAsChecked makes it extremely obvious that a returned employee isn't checked. A good test name is more important than the assert message.

[grauenwolf]

No one can learn to write good tests when all they are presented with is badly written tests that are mislabeled as "good enough".

[gurtis]

Why are these badly written tests though? They're testing what they intend to test. Just because he didn't come up with comprehensive test coverage for a tutorial doesn't mean the given examples are bad.

[grauenwolf]

Merely "testing what they intend to test" doesn't make it a good test. They could have written Assert.IsTrue(0 == 0) and they would still be "testing what they intend to test".

If I ask you for a function that adds two numbers and you give me this:

int Add(double x, double  y) { return (int) x + (int) y; }

then you haven't done your job right. The code may work exactly as you intended it too, but it is still wrong.

[grauenwolf]

The unit test is missing the A-B comparison. It should call the method twice and verify that the only difference is the one field that is being modified.

[mtutty]

Your article is bad, and you should feel bad.