13

Let's start with an example.

Let's say, I have a method called export that depends heavily on the DB schema. And by “depends heavily” I mean I know that adding a new column to a certain table often (very often) leads to the corresponding export method change (usually you should add the new field to the export data as well).

Programmers often forget to change export method since it's not really clear you should even look at this. My goal is to force programmer explicitly make a decision to determine whether he forgot to look at the export method or just don't want to add a field to the export data. And I'm looking for the design solution for this problem.

I have two ideas, but both of them have flaws.

Smart “Read all” wrapper

I can create the smart wrapper that makes sure all data is explicitly read.

Something like this:

def export():
    checker = AllReadChecker.new(table_row)

    name    = checker.get('name')
    surname = checker.get('surname')
              checker.ignore('age') # explicitly ignore the "age" field

    result = [name, surname] # or whatever

    checker.check_now() # check all is read

    return result

So, checker asserts if table_row contains another fields that were not read. But all this thing looks kind of heavy and (maybe) affects perfomance.

“Check that method” unittest

I can just create the unittest that remembers the last table schema and fails every time the table is changed. In that case programmer would see something like “don't forget to check out the export method”. To hide the warning programmer would (or wouldn't — that's a problem) check out export and manually (that's another problem) fix the test by adding new fields into it.

I have a few other ideas but they are too troublesome to implement or too difficult to understand (and I don't want the project to become a puzzle).


The above problem is just an example of the more wide class of problems I encounter from time to time. I want to bind some pieces of code and/or infrastructure so changing one of them immediately alerts programmer to check another one. Usually you have some simple tools like extracting common logic or writing reliable unittest, but I'm looking for the tool for more complex cases: maybe some design patterns I'm now aware of.

  • Can you maybe generate export based on the Schema? – coredump Sep 29 '15 at 15:55
  • It can't be aoutmatically generated, that's why I should ask programmer to look at the code and make a decision. – Vadim Pushtaev Sep 29 '15 at 15:59
  • Would it be a solution to add two lists of fieldnames (export and dont-export) to the export class, and have a unit test that checks that those two lists together encompass the full set of fields from the database? – Sjoerd Job Postmus Sep 29 '15 at 17:19
  • Can you autogenerate a test though that checks whether export has everything you realistically need? – biziclop Sep 29 '15 at 19:10
  • 1
    a comment in the source code too simplistic a solution? Usually things get missed because there's no reminder, a comment would fix that. – gbjbaanb Sep 30 '15 at 11:55
11

You're on a right track with your unit test idea, but your implementation is wrong.

If the export is related to the schema and the schema changed, there are two possible cases:

  • Either the export still works perfectly well, because it was unaffected by a slight change in the schema,

  • Or it breaks.

In both cases, it's the goal of the build to track down this possible regression. A bunch of tests—would it be integration tests, or system tests, or functional tests or something else—ensure that your export procedure works with the current schema, independently of the fact that it changed or not since the previous commit. If those tests pass, great. If they fail, this is a sign to the developer that he may have missed something, and a clear indication where to look.

Why is your implementation wrong? Well, for several reasons.

  1. It has nothing to do with unit tests...

  2. ... and, actually, it's not even a test.

  3. The worst part is that fixing the “test” requires to, well, actually changing the “test”, that is doing an operation which is completely unrelated to the export.

Instead, by making actual tests for export procedure, you make sure that the developer will fix the export.


More generally, when you encounter a situation where a change in one class always or usually requires a change in a completely different class, this is a good sign that you did your design wrong and are violating the Single Responsibility Principle.

While I talk specifically about classes, it applies more or less to other entities as well. For instance, a change in database schema should be either reflected automatically in your code, for instance through code generators used by many ORMs, or should at least be easily localized: if I add Description column to Product table and I use no ORMs or code generators, I at least expect to make a single change within Data.Product class of the DAL, without the need to search through all the code base and find some occurrences of Product class in, say, presentation layer.

If you can't reasonably restrict the change to one location (either because you're in a case where it simply doesn't work, or because it requires a huge amount of development), then you create a risk of regressions. When I change class A, and class B somewhere in the code base stops working, it's a regression.

Testing lowers the risk of regressions and, which is far more important, shows you the location of a regression. This is why when you know that changes in one location cause issues in a completely different part of the code base, make sure you have enough tests which raise alarms as soon as a regression appears at this level.

In all cases, avoid relying in such cases just on the comments. Something like:

// If you change the following line, make sure you also change the corresponding
// `measure` value in `Scaffolding.Builder`.

never works. Not only developers won't read it in most cases, but it will often end either removed or moved far away from the concerned line, and become impossible to understand.

  • Yes, that “test” is not a test indeed, it's some kind of trap, some kind of If you change the following line... that works automatically and cannot be simple ignored. I've never seen somebody actually used such traps hence the doubt. – Vadim Pushtaev Sep 29 '15 at 18:40
  • 1
    The problem is the class B doesn't stop working, it maybe starts working improperly. But I can't predict future and can't write tests that predict future, so I try to remind the developer to write that new test. – Vadim Pushtaev Sep 29 '15 at 18:42
  • @VadimPushtaev: when it comes to testing, “maybe starts working improperly” and “stops working” is exactly the same thing. You can't predict future, but you should be able to know exactly the requirements, and test that those requirements are fulfilled by your actual product. – Arseni Mourzenko Sep 29 '15 at 19:07
  • Yeah, that's a thing. I actually want to remind the developer to think about new requirements. I just want to wave a hand: “Hello, are you sure you don't forget about export? Ask the manager or something, it's a common problem”. – Vadim Pushtaev Sep 29 '15 at 19:19
  • Thank you anyway, your answer helped me to organize my thoughts and I have a certain plan now. – Vadim Pushtaev Sep 29 '15 at 21:48
3

It sounds to me like your changes are underspecified. Say you live somewhere that doesn't have postal codes, so you don't have a postal code column in the address table. Then postal codes are introduced, or you start dealing with customers who live where there are postal codes, and you have to add this column to the table.

If the work item just says "add postal code column to address table" then yes, the export will be broken, or at least won't export postal codes. But what about the input screen that is used to enter postal codes? The report that lists all the customers and their addresses? There are tons of things that need to change when you add this column. The job of remembering those things is an important one - you shouldn't be counting on random code artifacts to "trap" developers into remembering.

When the decision is made to add a meaningful column (that is, not just some cached total or denormalized lookup or other value that doesn't belong in an export or report or on an input screen) the work items created should include ALL the changes needed - adding the column, updating the populate script, updating the tests, updating the export, the reports, the input screens and so on. These may not all be assigned to (or picked up by) the same person, but they must all get done.

Sometimes developers choose to add columns themselves as part of implementing some larger change. For example someone may have written a work item to add something to an input screen and a report, without thinking about how it is implemented. If this happens a lot, you will need to decide whether your work-item-adder needs to know implementation details (so as to be able to add all the right work items) or whether developers need to be aware that the work-item-adder sometimes leaves things out. If it's the latter then you need a culture of "don't just change the schema; stop and think about what else that affects."

If there were a lot of developers and this happened more than once, I would set up a checkin alert for the team lead or other senior person to be alerted on schema changes. That person could then look for related work items to deal with the consequences of them schema change, and if the work items were missing, could not only add them but educate whoever left them out of the plan.

2

Almost always, when creating an export I also create a corresponding import. As I have other tests which fully populate the data structure being exported, I can then build a round-trip unit test which compares a fully populated original with an exported then imported copy. If they are the same, then the export/import is complete; if they are not the same then the unit test fails and I know that the export mechanism needs updating.

  • this can check, that every object is saved and reloaded from db. It does not cover the case that an existing db-field has no corresponding object field. – k3b Sep 30 '15 at 10:51
  • @k3b true. At least in my systems, that generally occurs if something is no longer used but hasn't been removed from the schema, which is outside of the scope of the export mechanism - the unit tests for object persistence check whether each field of an object is persisted, but I don't test for unused columns as that would have no observable effect on the system function. – Pete Kirkham Sep 30 '15 at 10:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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