85

A bit of context: Earlier today I had to update some SQL code that another colleague of mine provided, and since it’s a pretty large script, it’s stored as a separate file (which is then read and executed at runtime). While doing this I accidentally reintroduced two bugs we had a few months back, namely:

  • For whatever reason the ASCII file was encoded in UTF-16 (the colleague emailed me the file, which might have caused it).
  • The script was missing initial SET statements (required due to some driver things on production, but not on a clean install locally).

After debugging this for about an hour (again) I decided to write some unit tests to ensure this would never happen again (and include a quick way to fix it in the assertion message to provide an easy fix for future developers).

However when I pushed this code another colleague (who is also our team lead) walks up to me and told me I shouldn't make these things again because:

"These things don't belong in unit tests"

"Unit tests should only be used to check the flow of your code"

I’m pretty conflicted now since I still think what I’m doing isn’t wrong, as this bug wouldn’t be reintroduced in the future, however this colleague works as a senior and at the end of the day gets to decide what we spend our time on. What should I do? Am I wrong for doing it this way? Is it considered bad practice?

Improve this question
11
  • 7
    Possible duplicate of Should you write automated tests to check that data is correct (the contents of config files, databases, etc.)?
    – gnat
    Oct 31, 2017 at 7:08
  • 35
    "Unit tests should only be used to check the flow of your code" I'd say that's bullshit. Traditionally they should include all the tests necessary to ensure that the "unit" considered in isolation is correct. If you write only those unit tests that are useful to "check the flow", whatever that means, I hope you also have separate extensive testing suites (wrote by the QA department?).
    – gbr
    Oct 31, 2017 at 14:42
  • 8
    The problem of your colleague anyway is likely just where you put those tests. I'd focus on that, leaving aside denominations discussions/holy wars. It's possible that those tests are too slow for the suite you added them to, but it's also entirely possible that your colleague is just fixated on his idea of unit tests and is making a problem out of a non-existent issue; so it's better to first clarify what the real problem is.
    – gbr
    Oct 31, 2017 at 14:56
  • 2
    By the way, those tests do look like something you'd want to run every time you modify that SQL file. Here the main problem might be the testing tools, which might not support a "run only if modified" mode of operation; it that gives rise to real, concrete problems it might be worthwhile to include the "only if modified" functionality manually with some kludge just for those specific tests.
    – gbr
    Oct 31, 2017 at 14:56
  • 5
    Instead of testing that the file has the correct contents and encoding why not test that it works?
    – user253751
    Oct 31, 2017 at 21:28

12 Answers 12

Reset to default
154

Most likely the tests you wrote are closer to integration or regression tests than unit tests. While the line can be very fuzzy and sometimes devolves into pedantry over what is or is not a unit test, I would go back to your colleague and ask where the tests you wrote should be since they do add value ensuring correctness of the code.

I would not focus to much on what is or isn't a unit test and realize that even if its an integration test, there could still be value in the test.

Improve this answer
1
  • Comments are not for extended discussion; this conversation has been moved to chat.
    – Thomas Owens ♦
    Nov 5, 2017 at 17:20
35

Technically, it's not a unit test, and more of a validation step. The proper approach really depends on what your workflow needs to be. Your team lead is correct on what the purpose of unit tests are. My feeling is that this is a case of using the wrong tool for a job that still needs to be done. So start with this:

What's the problem I'm trying to solve?

By the description, you need to validate that any database scripts comply with some standards.

What tools/processes are available to solve the problem?

Source code quality is usually checked by static analysis tools. If you don't have a static analysis tool to validate your SQL, then you might create a quick and dirty tool that performs the check on any SQL file passed in to it. It doesn't hurt to check if there are static analysis tools that can handle the issues you are talking about.

If you make that part of your build infrastructure, such as incorporating it into Jenkins or something like that, it can be applied to all SQL files in your project.

The unit tests only solve the problem for your current file.

How do I communicate the need for the tool?

This is pretty easy, you talk to your team lead. He can work with the product owner and you to determine the risk/reward of investing in the tooling. If this is likely a one-off problem then the tooling would probably be overkill. If the tooling to catch the biggest issues is easy, it may be worth it just for the sanity check.

Your team lead may have some ideas that you (or I) haven't considered, that can address the problem more correctly.

Improve this answer
8
  • 21
    When discussing cost vs. risk, it would be important to note that it has already caused lost time due to reintroducing previously solved bugs. This alone is a strong argument to automate the verification. +1
    – jpmc26
    Oct 31, 2017 at 0:59
  • 1
    @jpmc26, I completely agree. The discussion should start with the fact that you lost however many hours figuring out what was wrong, and your unit tests were your first attempt to prevent the rest of the team from losing the same amount of time. However, working with the team lead who has to answer to management and stakeholders is usually very appreciated. As a team lead, I want to be able to defend the tools/practices/code we manage. If I saw unit tests that couldn't be traced to actual requirements I'd be concerned as well.
    – Berin Loritsch
    Oct 31, 2017 at 13:22
  • 1
    @BerinLoritsch Technically, it's not a unit test I would really like to know on what "technic" you base this assertion. As far as I can tell, there's no single authoritative definition of unit tests and everyone has come to have its own idea of what "they are".
    – gbr
    Oct 31, 2017 at 13:37
  • 2
    @gbr There is an informal agreement among developers that unit tests are tests that are run in isolation from external systems. They test only the code itself, not interactions with files, databases, or other external services. Wikipedia does document this understanding: en.wikipedia.org/wiki/Unit_testing#External_work.
    – jpmc26
    Oct 31, 2017 at 18:35
  • 1
    @BerinLoritsch It's also possible that we're all interpreting the question in a different way, anyhow, it was not very detailed and the author has not come back to anyone yet. I'm not very interested in discussing further about the classification of these tests anyway, what's important is if they should exist (I'm pretty sure they should) and how often to have them run (at every change in the IDE, at every local commit, at every push to the central repository, at every tot time...).
    – gbr
    Nov 2, 2017 at 17:34
19

It is bad practice to call tests that access files "Unit Tests".

He: "These things don't belong in unit tests"

You: "Makes sense, but I couldn't find a better place to put them. Where do they belong?"

Unfortunately, what kinds of tests exist and how they are organized is entirely company specific. So you'll need to find out how your company handles these tests.

If you don't yet have a way to run automated tests other than Unit Tests, the pragmatic approach is to mark the Unit Tests that are not actually Unit Tests with a prefix, until you have enough of them to start figuring out what kind of tests you actually have/need. After that you can start organizing.

Improve this answer
10
  • 2
    It is bad practice to call tests that access files "Unit Tests". Here the file that's being accessed is the source file. It will get accessed just as much as any source file (to parse it). The fact that the test will probably not be made in the same language of the "unit" being checked (SQL) makes it unorthodox but should not influence its classification as a unit test. continues...
    – gbr
    Oct 31, 2017 at 14:08
  • 1
    ... Actually, a file's correct encoding is a "test" that's made by any compiler every time it reads a source file. Here the problem is that being an external file interpreted at run-time the "compiler tests" will not be run automatically, and so it's entirely appropriate to add them explicitly, and I think it can be rightfully considered a "unit test" of that SQL snippet. And it does seem reasonable to include it in the (potential) suite of tests run at every modification of the file.
    – gbr
    Oct 31, 2017 at 14:08
  • 3
    By the way, what's generally recommended against is to access external files when that can be substituted with a mock or something along that way. And by most definitions unit tests can access external files or whatever, it's just strongly recommended against, as it can slow things down a lot. A shop is free to prescribe that you can't add tests that access files to the suite of tests that is run most frequently, but that doesn't make such tests unworthy of the "unit test" designation, they just make them "not to be put in the most frequently run tests' suite of this project".
    – gbr
    Oct 31, 2017 at 14:08
  • 2
    @Warbo It is a bad practice to access files (in general), and the (most important) reason is that they are not slow if they involve "GBs read over a flaky NFS link", they are slow if, e.g. quoting Michael Feathers, they take 1/10th of a second. That's because you want to run your tests as frequently as possible, ideally at every change you make in the IDE, and when you have a lot of them (as you should) even 10ths of seconds cumulate to hours. (continues...)
    – gbr
    Nov 2, 2017 at 18:03
  • 1
    @Warbo .. That said, what matters is the total time they take, so if you have a very small project that you're sure will stay small, you can be much more lenient about the speed of the individual tests. And if you really don't care to run them frequently, you're completely free to even have them call an OPMROC employee to grab and fax them a file from a cabinet. You can also choose to be more lax while you still have few tests and go back to speed them up when they start to take too much, but you must take into account that this is a debt that you're accumulating.
    – gbr
    Nov 2, 2017 at 18:03
14

Michael Feathers says this in his book Working Effectively With Legacy Code:

In the industry, people often go back and forth about whether particular tests are unit tests. [...] I go back to the two qualities: Does the test run fast? Can it help us localize errors quickly?

Will your test help localise errors quickly and run fast? If yes, then do it! If no, then don't! It's as simple as that!

That being said, you are working in an environment with other people and have to get along with them. You might have to end up doing it his way, even if you privately disagree with it.

Improve this answer
5
  • It's a good rule of thumb, but he would have spared confusion if he had wrote "whether to add particular tests to the most frequently run tests suite", rather then messing with the "unit test" term.
    – gbr
    Oct 31, 2017 at 14:24
  • 1
    @gbr If the results of the test are accurate, the only other thing that's important is how quickly it runs. If I have 100 tests that each run in under 0.1s, in total they'll run in less than 10s. I'm happy to run them frequently. If I have 1000 tests, they'll take up to 1m40s. That's too long, I won't run them frequently, but I'll run them once I've made the changes with the smaller group of 100 tests. I don't care if it's technically an acceptance test or something else. If if helps me find errors sooner, I'll do it regardless of semantics. A test only provides value when it's run.
    – CJ Dennis
    Nov 1, 2017 at 3:55
  • I mostly agree (independence is another very important thing, e.g.), but nonetheless, it would have been better if Michael Feathers hadn't compounded the confusion about what "unit test" means. Not that he is particularly to blame for that confusion (and his book is excellent, in the parts that I skimmed so far). I was making a rather minor point, anyway.
    – gbr
    Nov 1, 2017 at 18:13
  • @gbr He's not redefining unit tests, he's saying that shouldn't be your criterion for inclusion. Your criterion should be usefulness, and that is what he's defining.
    – CJ Dennis
    Nov 1, 2017 at 21:03
  • I re-read that section; I'm not sure, it's unclear to me if that's meant to be a (sort of) definition or just a criterion. But actually "Can it help us localize errors quickly" might very well imply the things that he says before, about isolation etc. . So I might have made a fuss about nothing (although your answer alone could still be misinterpreted)
    – gbr
    Nov 2, 2017 at 17:06
10

I have written similar tests, on occasion, against source-code files, configuration files, and so on. I wouldn't call them unit-tests because (a) they are accessing the file system and may not be ultra-fast (b) I don't care if they are executed on every check-in (as opposed to nightly on a CI server).

You might call them integration tests; certainly, they are closer to that perspective than unit tests.

My own term for them is resource tests. IMHO, they are entirely justified if executed nightly on a CI server: there is minimal cost and, when used judiciously, clearly add value. One definition of judiciously: if the test is checking an issue that caused a problem (such as the encoding that you mention).

Improve this answer
4

A unit test is all about testing a method or 'unit' of code. You're testing the smallest group of logic and code in your software.

Later, when you join that with other units you'll perform integration testing.

I hope your team lead encouraged your initiative and should have offered alternative suggestions. You definitely have the right idea.

Your SQL is code just like any lower generation language like C# or Java and should be tested as such. And verification and validation belong to all testing levels. So encoding and SET statements are included, but not necessarily tested exclusively. General stuff like line endings or enclosing you can usually just use an SCM hook or feature.

Best practice is to have regression tests to ensure that past bugs don't get reintroduced. Generally, tests are created alongside any resolution of the bug. If these bugs are not covered by regression tests on unit/integration or system level and then reintroduced it's a team problem, a process problem, not an individual one.

The thing is... syntax errors, missing statements or logic blocks inside a 'unit' don't typically get tested. You are testing the inputs and outputs of the unit in different combinations, testing the many possibilities that could be generated.

Getting back to missing SET statements - they help inform the many possibilities of input and output to test for. What test would you write that would FAIL if you were missing any chosen SET?

Improve this answer
2
  • "Unit of code" testing is one approach to unit testing. In my experience this leads to fragile tests and massive bloat (e.g. excessive mocking). An alternative, and IMHO better, approach to unit testing is "unit of functionality". It doesn't matter if some functionality (say, "setting a cookie when logged in") requires one method or a dozen intercommunicating processes, it's still one unit.
    – Warbo
    Nov 1, 2017 at 21:31
  • @Warbo - I would call that closer to (but not) integration testing. Unit testing doesn't require excessive or massive anything. Unit tests should be small and fast. Actually testing by functionality leads to what you describe .. Fragile tests are ones that are 1. larger or do more than they should. 2. highly coupled 3. don't have a single responsibility.
    – Ross
    Nov 12, 2017 at 22:22
3

If you have files that become part of your product, then their contents must be correct. No reason why you wouldn’t verify this. For example if you need six 1024x 1024 images in some folder, then by all means write a unit test that checks you have exactly that.

But you probably don't just have the files, you also have some code that reads the files. You could write a unit test for that code. In the example above, does the function to read one of the six images return a 1024 x 1024 image in memory (or whatever it was supposed to produce).

Anyway, it may not be a unit test, but it is a useful test. And if you use a unit test framework that allows you to do a useful test (that is not a unit test), why not use the unit test framework?

Improve this answer
2

Perhaps I'm misunderstanding your issue, but to me this sounds like a problem that should not need to be captured by any sort of dedicated test but simply by the version control system. Any change to a codebase should be reviewed on a patch-by-patch basis before committing. A simple way to do this in git is to add the changes with

git add -p

This will for each change in a text file the working directory ask you whether you really want to keep it. That would allow you to see, for instance, the deletion of those “initial SET statements”.

In case the encoding of an entire file changed, somthing different would happen though: the algorithm would fail to diff old and new file, and therefore git add -p would not add anything at all. This would then be visible in the other command I'd do before any commit, namely

git status

Here you'd see the file highlighted in red, indicating that there are changes. Investigating why these didn't make it into git add -p would quickly make the problem obvious.

Improve this answer
6
  • pray tell, how does this help any future dev to avoid exactly the same problem? ... the thing about automated tests (also valid about assertions and design-by-contract) is that they are, well, erm, automated.
    – user88637
    Nov 3, 2017 at 5:24
  • @vaxquis it does prevent the exact same problem – though somewhat coincidentally, as a side-effect of a workflow that's a good idea for different reasons. My point is, that this problem could happen at all shows the OP's team didn't make very good use of their VCS. — Nothing against automated tests, but their value is testing semantic properties which might break by innocuous changes to the program logic. It is not to check every possible stupid way in which the source code might change.
    – leftaroundabout
    Nov 3, 2017 at 9:57
  • by your logic, we don't need seat belts; we just need to drive more carefully and cause less accidents... You missed the main point OP raised - that of a human error. No amount of VCS can protect you from that. Also, FWIW: if a test can be automated, it should be automated. Humans are always the weakest links in engineering processes. git is the best example of that - a great tool, yet barely unusable for mere mortals.
    – user88637
    Nov 3, 2017 at 10:11
  • @vaxquis No, no! Seat belts are analogous to the kind of tests that make sense: they catch a wide array of situations that are likely to occur by accident. A test on file-encoding would be analogous to a robot that follows you around and prevents you from asphyxiating yourself by stuffing beans up your nose.
    – leftaroundabout
    Nov 3, 2017 at 10:23
  • According to the OP, files in the wrong format have happened twice already, so apparently they are likely to occur by accident.
    – gnasher729
    Nov 5, 2017 at 18:59
1

Another angle to consider: since those two conditions are requirements for your program to run, shouldn't you embed the logic near to the execution logic? I mean: you test the existance of a file before reading it and/or validate it's content, right? so how this is different? I think that since this is a code-external resource, it should be validated at runtime, before it's actually used. Result: stronger app, no need to write additional tests.

Improve this answer
9
  • 1
    How does failing only at runtime make it a stronger app? Sure, it can be appropriate to also have runtime checks close to the source of the problem, but if you can detect mistakes before they cause runtime problems, it's a lot better, don't you think? Are you sure you're familiar with automatic testing?
    – gbr
    Oct 31, 2017 at 14:18
  • 1
    "How does failing only at runtime make it a stronger app?" Throw an exception if the check fails. In your test, just check that the section of code being tested returns the expected result: this remove the burden to check one more reason for the failing.
    – Dr. Gianluigi Zane Zanettini
    Oct 31, 2017 at 15:23
  • 1
    Your strategy has (almost) nothing to do with unit testing and automated testing in general, it's a different thing, with different uses.
    – gbr
    Nov 1, 2017 at 16:13
  • 1
    I was going to suggest this. The bug is an implementation detail; I imagine the responses would be very different if the dodgy encoding was in a private field rather than a standalone file! It sounds like OP has 2 problems: resource files might be badly encoded, and production behaves differently to dev. By checking the file at runtime, just before it's used, we solve the second problem: dev and prod will throw the same error. Unit tests can then focus on the actual functionality rather than the implementation details; these "internal" checks will be exercised just like private methods.
    – Warbo
    Nov 1, 2017 at 21:42
  • 1
    @Dr.GianluigiZaneZanettini Bah... I give up... As I see it, at best your answer, after your "clarifications" in the comments, was off-topic (not an answer to the question), but in reality, as it stands, it's plain wrong! no need to write additional tests ??? I don't have enough reputation to downvote it, but consider as if I did it. And I don't think there's much value in continuing this conversation.
    – gbr
    Nov 2, 2017 at 15:18
1

Tests are the same code as any other and, if complex enough, also benefit from ... unit testing. Seems simplest to add such precondition checks directly into the test.

Most of the tests are simple enough not to require this, but if some are sufficiently complex, I do not see anything fundamentally wrong with these pre-condition checks. Of course, the test should also fail without them, but a good unit test also tells which unit is failing.

A script that is used as part of the test and must have certain content and encoding is probably a unit. It may have much more code and logic than the rest of the test. A test with such script is not the best design ever and, if possible, should be refactored into something more direct (unless this is integration test).

Improve this answer
4
  • 1
    the author doesn't say anywhere that that SQL script is a part of some test, you seem to have misread the question
    – gbr
    Nov 2, 2017 at 18:24
  • There difficult to understand, I assume the SQL script is part of the test.
    – h22
    Nov 2, 2017 at 18:33
  • your comment "There difficult to understand"...
    – gbr
    Nov 2, 2017 at 18:38
  • Difficult to understand. Downvoting the question.
    – h22
    Nov 3, 2017 at 7:10
1

Firstly - one of the purposes of tests is to prevent issues from recurring in your code - so you absolutely should keep writing tests of this nature.

Secondly - naming is hard. Yes, these are clearly not "unit tests", but they can be desirable and necessary parts of the build process, because they protect you from obvious mistakes, and because they give you feedback about errors sooner (especially given you don't see the consequences on a dev box).

So, the question really is (should be in your context) more about when and how these tests are run than what they are.

I've used this sort of test extensively in the past - they've saved us a fair chunk of pain.

Improve this answer
1
  • And if someone would care to explain the downvote I'd appreciate it
    – Murph
    Nov 4, 2017 at 15:26
1

Unit tests are about executing a unit of code in isolation to confirm that it is producing the correct result for the correct input. Isolation should make both the unit under test and the test itself repeatable i.e. should not depend on or introduce side effects.

SQL isn't exactly something that can be tested in isolation, so any test of SQL isn't exactly a unit test, and, except for SELECT statements, is almost certain to have a side effect. We can call it an integration test rather than a unit test.

It is always wise to ensure that any defect which could be introduced can be detected as early as possible in the development cycle, and beneficial to do so in a way that makes it easy to identify the source of the defect so that it can be quickly corrected.

The tests in question may be more appropriately relocated out of the body of "unit tests" and placed somewhere else, but should not be removed altogether if they are doing something useful like guarding against the possible introduction of a defect which could take hours to track down.

Improve this answer

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