In testing is making asserts in cleanup bad?

Question

As a learning excercise, I decided to take a hand on Test Driven Development.

Now I decided that there is a test I want to make; check if database connection doesn't leave any unsaved changes. The two options I can easily see are:

• I could put the assertion at end of every test method.
• I could add the assertion in cleanup which is ran after every test method.

I was initially considering to put the test at end of every test method, but that leaves a lot of code duplication, and could be missed out. On the other hand, putting assertion in cleanup ensures that the database never has unsaved changes, however it feels a like it violates Single-Responsibility of the cleanup function (which is to clean up resources).

        [TestCleanup]
        public void Cleanup()
        {
            Assert.IsFalse(dbContext.ChangeTracker.HasChanges(), "unsaved database changes detected.");

            authContext.Dispose();
            dbConn.Dispose();
        }

This makes me wonder in testing is making asserts in cleanup a bad idea?

Answer 1

I am less concerned about DRY code in unit tests, and more concerned about the test clearly stating the intended behavior. Doc Brown has sensible advice, but I would prefer the assert in each test.

If having no unsaved changes is one of the expected behaviors of the system under test, put the assert in the test. Yes, it is a repeated line of code, but refactoring it into a private method can clean that up. The important thing is the test clearly indicates that having no unsaved changes is the desired behavior, and this fact is traceable from the test itself. It makes the test easier to understand later when you come back and read it, because you don't need to remember that the cleanup code is magically called by the test framework. You can see from the test exactly how the control flow leads to the failure.

I've written tests that performed asserts in a cleanup method. While I could trace through code to identify which test case was failing, it was always a jarring experience. The failure wasn't immediately obvious, because the stack trace referenced methods from the test framework itself in between the failure and the test method.

Placing asserts in the test cleanup might appear safe, and might not appear to interfere with disposing of resources, but I'm always leary about purposefully throwing exceptions in cleanup code. A change in some other location of the application or test could prevent resources from being disposed of properly. This can cause a cascade of failures that are unrelated to the original failure, which complicates the trouble-shooting process. For this reason alone I like to keep asserts and cleanup code separate, so that one does not affect the other.

Think of it as adhering to the Single Responsibility Principal in your test code. The system under test and the cleanup are separate concerns, and change for different reasons. They have different responsibilities, and therefore deserve different treatments in your test code.

Answer 2

This is ok as long as

• your test runner lets you still see easily which specific test caused the assertion to fail

• the failing assertion does not prevent the cleanup of the resources up to the point where other tests start to fail exactly because of the latter (of course, this should not happen for failing asserts directly in the test functions as well)

But you should be sure that within the category of tests in this class for all of them having no unsaved changes is really the correct requirement to check.

Otherwise, consider the alternative of putting the "Assert" line into a reusable function and call that function at the end of each test where it makes sense. That might require a more boilerplate code, but does not duplicate any logic, only the calls to a common logic, which is a far more acceptable form of non-DRY code.

Answer 3

Just make sure that (a) the failing assertion is detected, and (b) your unit testing software can handle this - it may not expect assertions during the cleanup and misbehave in some way.

So with your application having a small number of known test failures, add Assert.isFalse(True) to the cleanup code, and check that every cleanup fails, and you still have the known failures.