3

Suppose I had some Manager class that I need to change in regards to existing functionality by removing code.

The Manager always sends an initial message after a connection was established to do an operation e.g. clearInitialData(). I have to remove this initial sending.

There exist about 30 tests in our unittest suite on this class. Unittest style is kind of integration tests but with mocks.

However, every single unittest except testCreation() depends on this test case "testInitialDataCleared()" which tests that the initial sending gets done. However that is exactly the behavior I need to remove.

I had a bit trouble in how to get started on this, so my approach was:

  1. make new test that ensures that the initial data cleared doesnt get done

  2. removed 2 obviously obsolete test cases because they were just testing this kind of initial clearing step

  3. Run all tests. As expected, my own new test fails where the old tests were passing.

  4. Edit the existing tests to depend on my new test case that I created. It seems the reasonable thing to do because previously those tests started with given assumption that there would have been initial clearing sent

  5. See more fails from tests

  6. Make what I think will be the implementation fix. Removing code from implementation.

  7. Now I am seeing assertions failing from business logic, and all test cases won't finish running anymore due to core dumped from assert (using google test / mock btw).

Not sure if this was best TDD practice, maybe my implementation fix was a bit too intrusive. Likely I will need to run the test in debugger anyway to figure out what the hell went wrong.

Have you ever had similar issues with development and do you have any good tips about it?

conclusion took a look at it with collegue. Actual business logic implementation removal was pretty much solid. What was causing test assert was that the uut did have static state that got maintained. And my new unittest ran the uut into an unexpected state so that one of the other tests had this assertion. So basically I had forgotten to do a reset at end of my new added unit test due to static data. So basically i had made the test as separate test case in google test which was causing issue.

on unittest side we went with the idea of having approach of replacing all testInitialDataCleared() with test helper that checked nothing was done. Couple old behavior unittests clearly removed.

4
  • 3
    In my view, if the tests depend on each other, then they're not really unit tests at all. Commented May 5 at 10:01
  • 1
    Tests aside, it seems like the logic would run differently if "clearInitialData" isn't called? it implies some shared state. Are you sure all you need to do is remove the clear call?
    – Ewan
    Commented May 5 at 12:44
  • 1
    @Ewan: that's precisely what my answer is saying, thanks there is at least one person here who reads the question the same way as me.
    – Doc Brown
    Commented May 5 at 15:33
  • 1
    yeah the question gets muddled after (5), did they make the correct change? is it the tests or BL thats erroring out? Was there an Issue writing the tests or with TDD? people are picking up on the shared test, but it doesn't seem like it actually caused an issue in this case?
    – Ewan
    Commented May 6 at 11:12

3 Answers 3

7

You are feeling the pain that happens when tests are not written to be independent. Step one is making all of these tests independent. Break the dependency between testInitialDataCleared and all the other tests. Odds are all tests call the first one in order to simplify the setup for each test.

Consider moving the minimal common test setup logic into a reusable utility method. I would advise against calling this common setup method inside testInitialDataCleared just to keep the first test independent. Besides, you want to remove that test anyways.

It is fine for tests to have a common setup routine. It's not good when the first test is the setup routine. Once the tests are independent, remove the code in your system under test, followed by removing the obsolete tests and changing the common setup for the remaining tests. The rest of the job should go easier and reveal itself after that.

As you add, remove, and change tests, keep in mind that a unit test is defined by the requirement being tested, the smallest unit of code necessary to implement the requirement, and the minimum number of assertions needed to ensure the test fails if the unit of code no longer implements the requirement.

4
  • Reading the question twice, I doubt testInitialDataCleared contains common setup logic for the other tests which can be easily extracted. I guess though the OP called it a "test", it is just a helper method which validates the invariant "Initial data was cleared" in each of the other tests.
    – Doc Brown
    Commented May 5 at 6:02
  • @DocBrown, how does validating the invariant "initial test data cleared" differ from testing that the initial data is cleared? Further on, the OP states that all tests depend on testInitialDataCleared, except one. If it walks like a duck and quacks like a duck, it's a duck. And this is walking and quacking like one test is part of the setup for the other tests, which is causing headaches with removing behavior and changing tests. Commented May 5 at 12:52
  • 1
    Well, you wrote "It's not good when the first test is the setup routine.". I doubt this is the OPs case. "testInitialDataCleared" isn't called because of some common setup logic, I believe it is called to verify that the initial data was cleared in each use case described by the other tests. Hence it is ok to have a reusable method in the automated tests which tests the invariant. Bur by renaming it to "validateInitialDataWasCleared" and not using it as a standalone test itself won't fix the OPs problem. ....
    – Doc Brown
    Commented May 5 at 14:10
  • ... I guess the real problem isn't the dependency of the "use case tests" from "testInitialDataCleared", but the dependency of the BL code from the effects of clearInitialData - that are the dependencies which must be broken.
    – Doc Brown
    Commented May 5 at 14:21
2

I think it is hard to give you reasonable advice without knowing the whole system, but my best guess is that you changed the Manager class to not call clearInitialData() anymore, and your test suite helped you to achieve this goal correctly.

So far, so good. However. sending clearInitialData() surely had a purpose for the business logic - just removing it will probably cause unwanted side effects which will only become apparent when the business logic gets run.

So what you need to understand first and foremost is to understand what removing clearInitialData() will mean for the BL part of your code. Ideally, the business logic part (in isolation) will work correctly with or without clearInitialData() called before, the decision whether it is called or not is done at a different level in your code and should be kept orthogonal to the BL itself (including all tests of the BL). The fact the assertions now fail show the BL currently isn't designed that way.

If you want to approach this by TDD, what you need to write here first is

  • one or more unit tests which run the affected business logic without the Manager class

  • a parameter to let the test setup control whether clearInitialData() is called before or not (it does not have to be clearInitialData() itself, but some equivalent method which simulates the effect from the point of view of the business logic code)

Then you can use this test in TDD style to prepare the business logic for the upcoming change. When you are done with this, then it is time to start changing the Manager class and it's testsuite, which should ideally result in a more successful outcome.

6
  • Assertions that fail are not related to business logic outside of Manager class, so testing of external classes is meaningless.
    – Basilevs
    Commented May 5 at 10:26
  • @Basilevs: the OP wrote "Now I am seeing assertions failing from business logic," - I don't know how you interpreted this sentence, but for me it is pretty clear these are assertions related to business logic.
    – Doc Brown
    Commented May 5 at 12:09
  • I assume those are not test code but are internal consistency checks like preconditions and invariants.
    – Basilevs
    Commented May 5 at 12:14
  • 1
    @Basilevs: so you say internal consistency checks like preconditions and invariants inside business logic are unrelated to the business logic? Does not convince me,
    – Doc Brown
    Commented May 5 at 12:17
  • 1
    These errors are only tangentially related to modification of Manager and will most likely fix themselves, once Manager and test environment are in working order. They are not subjects of the failing tests.
    – Basilevs
    Commented May 5 at 12:19
-3

Most of your failing tests are making an irrelevant assertion. Fix that before performing the change.

The test should assert whatever is relevant for the use case it emulates. As you have test failures removing irrelevant code, the tests were doing unnecessary assertions. Fix the tests to stay focused on their own scenario and result, without asserting unnecessary environmental details.

Unrelated Unit test tangent

There is an opinion in the industry that each unit test should make exactly one assertion. The stated benefit is that a failure reason is clear at a glance and error is easier to localize. The reasoning is invalid, as the first assertion to fail is the only one that matters when investigating a test failure.

In your case, the problem is not multiple assertions, but assertions that are not related to the test scenario.

Anyway, this is unrelated, as we deal with an integration test and opinions on integration tests are less strict and mostly less stupid.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.