• Sack (cs)

    Did they need to write a certain amount of tests to get management approval?

    There's the genius that writes a tool to automatically write tests in order to maximize a questionable productivity metric, but there are also clueless idiots who genuinely believe tests like the ones in the article add value.

  • RFoxmich (unregistered)

    So you might say they stubbed their toes?

  • Tatr (unregistered)

    I dont' nedd nos speofelchekcn or pewoeriviw beucaue i nwiow hwo owt tyep!!

    Submut

  • Roby McAndrew (cs)

    ArrayList is a very important library function, and it's good to know it is being thoroughly tested.

  • csrster (unregistered)

    Stubbing dependencies: good Stubbing the class you're actually testing: not so good

  • Lol (unregistered)
    A newly developed system was [...] tested to obscene levels

    I wonder what kind of input they fed the unit tests !

  • Roboticus (unregistered) in reply to Lol

    I suspect: N, S, F, W, true, false, FILE_NOT_FOUND.

    Possibly also "nulla", the captcha.

  • Anom nom nom (unregistered)

    On the bright side, tests will never break when someone modify the DAO's.

  • faoileag (unregistered)

    A wonderful WTF indeed! Looks as if the coder has no real knowledge whatsoever about what he is doing.

    So he knows about stubbing. Great. But he does not know (or does not care) that what you stub in such a case is Connection and PreparedStatement so that "ps.executeQuery();" delivers a suitable List depending on the statement in the sql field.

    Connection and PreparedStatement are both interfaces, Java really makes things easy for you here.

    But the reason they started to stub at all is a wtf in it's own right: "the developers said that the tests took too long to run when they hit the database" So, if the tests completed reasonably fast, they would have actually queried a database from their unit tests?

    That shows once again that unit tests might look simple but are sometimes hard to get right.

  • dtech (cs)

    Well this just again shows that just unit testing is not enough, you also need integration testing and should be careful when stubbing dependencies.

  • lol (unregistered) in reply to csrster

    Its called mocking, and its used extensively in all unit tests.

    And no, proponents of unit test don't like it when you use the term in its original meaning when referring to unit testing.

  • Cujo (unregistered)

    Garbage In, Gospel Out

  • faoileag (unregistered) in reply to Sack
    Sack:
    Did they need to write a certain amount of tests to get management approval?
    Probably yes - not in absolute numbers, of course, but in % code coverage. With unit tests you should definitely aim for 100% code coverage, so I wouldn't be surprised if such a directive existed.
    Sack:
    There's the genius that writes a tool to automatically write tests in order to maximize a questionable productivity metric, but there are also clueless idiots who genuinely believe tests like the ones in the article add value.
    Well, I wouldn't call 100% code coverage a "questionable" metric but rather "something we should definitely aim for".

    The test in the article does not add value, indeed. But that's because it has been implemented in a wrong way.

    A correct implementation would have stubbed Connection and PreparedStatement to match any sql statements that might occur in the DAOs. That way, you could get your 100% code coverage of the DAOs and, for instance, would catch any changes to the sql strings (like an accidentally inserted letter or something like that).

  • Smug Unix User (unregistered)

    Unit Tests don't magically transform bad programmers into good programmers. Programmers who take the time to actually understand what they are doing can use unit tests to verify the code still works the way they think it should.

  • RonBeck62 (cs)

    "When queried, the developers said that the tests took too long to run when they hit the database, so they created stub-DAO's to return canned data, because they assumed the SQL queries would be correct and didn't need testing."

    So, what did they think would happen when they went to production and the transaction volume started to actually tax the resources of the back end?

  • gtg (unregistered)

    It's true that unit tests should not run against your main database. Either you use a local test database, or you mock out the database access code.

    Mocking out the DAOs by returning canned data is quite reasonable when testing the rest of the code (business logic, etc). The only problem is that is leaves a hole in your testing, because you haven't verified the DAOs themselves.

    So TRWTF here is not measuring code coverage - which would have shown up 'the DAO code never runs as part of the tests'.

  • belzebub (unregistered)

    Tests are testing the code. Good. But what's testing the tests? I demand tests written to test the tests!

  • Matteo (unregistered)

    bool trueness = true; if (trueness == trueness) { return true; } else { return trueness; }

  • faoileag (unregistered) in reply to belzebub
    belzebub:
    Tests are testing the code. Good. But what's testing the tests? I demand tests written to test the tests!
    To be completely on the safe side, of course you also need to test the tests that test the tests.
  • KattMan (cs) in reply to faoileag
    faoileag:
    belzebub:
    Tests are testing the code. Good. But what's testing the tests? I demand tests written to test the tests!
    To be completely on the safe side, of course you also need to test the tests that test the tests.
    I use faith testing, my code is perfect, I have faith in that. Push a button and it just works, if you think it doesn't you just don't have enough faith.
  • mangobrain (unregistered) in reply to faoileag
    faoileag:
    But he does not know (or does not care) that what you stub in such a case is Connection and PreparedStatement so that "ps.executeQuery();" delivers a suitable List depending on the statement in the sql field.

    I don't have experience with the PreparedStatement interface, so I may be missing something here, but in order to stub it realistically - and actually test the SQL as written, instead of simply returning canned data from one layer down - wouldn't you effectively need to write an SQL interpreter? Which is itself a large body of code, warranting its own tests...

    A stub PreparedStatement interface returning a canned ResultSet still doesn't address the critical failure here: that the SQL statements in the code weren't tested against the target RDBMS. Even a very full-featured stub, wrapping a "real" DB such as SQLite backed by a local file, doesn't test the SQL dialect to be used in production against the RDBMS to be used in production.

    Perhaps unit testing is the wrong approach for a DAO, and it really does need to be integration tested in conjunction with a test DB instance.

  • faoileag (unregistered) in reply to mangobrain
    mangobrain:
    faoileag:
    But he does not know (or does not care) that what you stub in such a case is Connection and PreparedStatement so that "ps.executeQuery();" delivers a suitable List depending on the statement in the sql field.

    I don't have experience with the PreparedStatement interface, so I may be missing something here, but in order to stub it realistically - and actually test the SQL as written, instead of simply returning canned data from one layer down - wouldn't you effectively need to write an SQL interpreter?

    No, not really. There will be one sql statement per DAO, so in a stub (or mock) version of PreparedStatement you would basically test by string comparison against each statement and return a plausible set of results.

    In cases where variables play a role (e.g. in the WHERE clause), you either compare by regex (more generic apporach) or you test against a few literal versions of the DAO's sql statement with the variable value hardcoded in.

    You don't really need that much tests against a DAO - one or two that actually deliver data, one that doesn't and one that would produce an sql error. After all, you are testing the DAO here, not the database itself.

    So in the end, your PreparedStatement will feature a long if..else cascade, but that's ok, as long as each block has a comment stating to which DAO the block belongs.

  • mangobrain (unregistered) in reply to faoileag
    faoileag:
    There will be one sql statement per DAO, so in a stub (or mock) version of PreparedStatement you would basically test by string comparison against each statement and return a plausible set of results.

    That's not really any better than what is presented in the article. It still doesn't address the issue that the SQL statements themselves may be incorrect. OK, so you are now testing that the DAO can correctly extract data from the ResultSet into its return type, which is meaningful - but you're still force-feeding it the correct ResultSet.

    Arguably that may suffice for a unit test, but it still makes me cringe, and I still say in a real-world scenario there would need to be integration testing against the target RDBMS.

  • anonymous (unregistered) in reply to RonBeck62
    RonBeck62:
    "When queried, the developers said that the tests took too long to run when they hit the database, so they created stub-DAO's to return canned data, because they assumed the SQL queries would be correct and didn't need testing."

    So, what did they think would happen when they went to production and the transaction volume started to actually tax the resources of the back end?

    That's why the test server is an obsolete model with half the memory and speed of the ones they'll deploy it on. It'll magically just work on the better server.

  • Coyne (cs)

    Fast testing theorem: Presumption of correctness approaches infinity as testing approaches zero.

  • Walky_one (unregistered)

    I'd say the TRWTF is to presume that "Unit tests are OK" means "This code works correctly".

    Unit-testing is a very nice feature (if used correctly). But it doesn't replace a regular test. It's too easy to "stub out" some complex feature that suddenly behaves completely different in a complex environment (Let's talk about multi-threading, deadlocks and performance issues...)

  • faoileag (unregistered) in reply to mangobrain
    mangobrain:
    faoileag:
    There will be one sql statement per DAO, so in a stub (or mock) version of PreparedStatement you would basically test by string comparison against each statement and return a plausible set of results.
    That's not really any better than what is presented in the article.
    Is that so? In the article, what is tested is ArrayList's get() method. Does ArrayList's get() method need testing? No, I think I can trust the language in that respect.

    If you mock PreparedStatement, however you test the full DAO including the while loop in its getData() method. Especially the latter is important, since it populates getData()'s return value.

    mangobrain:
    It still doesn't address the issue that the SQL statements themselves may be incorrect
    A mocked PreparedStatement provides the possibility to detect syntax errors in the DAO's sql statement like an accidentally inserted letter or something, and it allows you to detect intentional changes as well. The mocked PreparedStatement so serves as the reference for the sql statements. You change them just for the fun of it, your unit tests croak. Testing sql statements against their database schemata is somewhat beyond the scope of unit tests, but if someone provides a validateStmntAgainstSchema(stmnt, schema) function even that could be made a unit test.
    mangobrain:
    OK, so you are now testing that the DAO can correctly extract data from the ResultSet into its return type, which is meaningful - but you're still force-feeding it the correct ResultSet.
    It doesn't matter what data the ResultSet contains, as long as it does contain data.

    This is a unit test of the DAO, not a test of the database or of what it contains.

    Some typical test cases would be:

    • does getData() return a List the same size as the ResultSet delivered by my mock PreparedStatement?
    • does getData() return an empty List if ps.executeQuery() returns an empty ResultSet?
    • does the DAO throw an exception if con.prepareStatement(sql) or ps.executeQuery() throw one?
    mangobrain:
    Arguably that may suffice for a unit test, but it still makes me cringe, and I still say in a real-world scenario there would need to be integration testing against the target RDBMS.
    And that's what this article is all about: a badly implemented unit test that gives a false sense of security, and a missing functional test against a real database before deployment.

    Unit tests are not a replacement for functional tests and even less for user acceptance tests; they are mainly meant for developers to get instant feedback the moment they break something.

  • Marco (unregistered)

    The problem here is just lack of knowledge from whoever wrote those tests.

    1st - When you're connecting to an external API, you write an adapter. You mock that adapter in your unit tests, and test the adapter with integration tests. Here they did neither.

    2nd - You don't mock your unit under test, that's not called testing, it's called wasting time. When you do this you're testing nothing, your code is not even running you're just testing the mocks themselves, you're clearing doing a favour to the author of the mocking library, by making sure his mocks work.

  • no laughing matter (cs) in reply to faoileag
    faoileag:
    No, not really. There will be one sql statement per DAO, ...
    This statement necessarily includes:
    SELECT ... ; INSERT ...; UPDATE... ; DELETE ...; DROP TABLE Students
    
  • Andres (unregistered) in reply to faoileag

    For the most part I think this approach is wrong and add very little value.

    You could have a valid but complicated SQL statement. Let's say it's running fine, but slow. You have a unit test verifying it. You go in and optimize the query. And then...

    If you have a mocked PreparedStatement that returns a mocked ResultSet based on the SQL and parameters passed in... your test is failing!! Even if the query is running correctly. You get the same failure if you insert a typo into the query, as if you optimized the query. Sure, you could then update the test, but that's a lot of work. And the unit test is still not giving you the information you need, which is whether the new SQL query is still returning valid data.

    I think the only way to test DAO is against a real database that is kept in a known state.

  • faoileag (unregistered) in reply to Andres
    Andres:
    You could have a valid but complicated SQL statement. Let's say it's running fine, but slow. You have a unit test verifying it. You go in and optimize the query. And then...

    If you have a mocked PreparedStatement that returns a mocked ResultSet based on the SQL and parameters passed in... your test is failing!! Even if the query is running correctly. You get the same failure if you insert a typo into the query, as if you optimized the query. Sure, you could then update the test, but that's a lot of work.

    If you unit test the sql statements used in your DAO, then yes, you will have to update the relevant unit tests once the statements change.

    If signatures or return values of your object's methods change, you will have to update you unit tests. Fullstop.

    "but that's a lot of work" is a lousy argument. "Did you test that release?" "No, too much work." "Ok, doesn't matter, it's just customer data we are loosing if anything goes wrong".

    Andres:
    And the unit test is still not giving you the information you need, which is whether the new SQL query is still returning valid data.
    What is so difficult to understand about unit tests? Unit tests verify the basic behaviour of an object, nothing more. getData() should return a List the same size as the ResultSet returned by executeQuery(). What data is in the list is irrelevant (for now).
    Andres:
    I think the only way to test DAO is against a real database that is kept in a known state.
    Than you should perhaps think again or dig a bit deeper into what unit tests are, and what not.

    Of course you should also test your DAO against a decent database. But not as part of a suite of unit tests.

    Here is a somewhat ideal testing scenario: Unit tests: on every submit Functional tests: if possible automated on a nightly build. Application-as-a-whole test: by your human tester as soon as he can test something. Say, pre-alpha. User acceptance test: if you work for a customer done by that customer once you are confident your software will pass the UAT.

  • snoofle (cs) in reply to Andres
    Andres:
    For the most part I think this approach is wrong and add very little value.

    You could have a valid but complicated SQL statement. Let's say it's running fine, but slow. You have a unit test verifying it. You go in and optimize the query. And then...

    If you have a mocked PreparedStatement that returns a mocked ResultSet based on the SQL and parameters passed in... your test is failing!! Even if the query is running correctly. You get the same failure if you insert a typo into the query, as if you optimized the query. Sure, you could then update the test, but that's a lot of work. And the unit test is still not giving you the information you need, which is whether the new SQL query is still returning valid data.

    I think the only way to test DAO is against a real database that is kept in a known state.

    to this end, we use an in-memory db (bitronix), which lets you set up your test by creating a real table and populating it with good and bad data, then using it as your data source, and physically running the DAO, checking the results against expected values. Then, your teardown method drops the test table.

    It's no substitute for performance or integration testing, but it lets you actually run your query without a real database.

  • mott555 (cs)

    My eyes glazed over while reading, and I didn't really see anything WTF-worthy. I figured it was either a bad WTF or I'm TRWTF. Then I saw the last few lines and almost got a concussion myself from the ensuing headdesk moment.

  • ONELINER (unregistered)

    Database DAO DOA.

  • jaffa creole (unregistered)

    I'm going to go out on a limb and guess that the other "unit tests" in the system were hitting the full stack. Some otherwise-smart guy tasked with making them run faster replaced all the database queries via some find and replace magic. Didn't pay too much attention to the fact that he was negating some tests. I've seen that one happen before.

  • Geoff (unregistered) in reply to gtg

    It also kicks the door open 10 times wider for the good old fashion "not a representative data set fail"

  • herby (cs)

    Unit tests ARE wonderful, unless all the units are different.

    My example: My brother generated MAC address roms for some vendors Ethernet cards. Now these MAC addresses MUST be unique, so he was THE vendor of the particular part (it was something like a $.30 chip, but programmed it went up to someting around $2.00 or so). The contract assembler of the Ethernet boards was also in charge of parts procurement and wanted to get good prices. They ignored the "sole source" notation on the MAC address chip, and just looked under the label and copied the same data into all of the chips for all of the boards. The end result was that all the Ethernet boards had the SAME MAC address. This is a REAL BAD thing. Of course they all individually passed unit tests.

    Then the seller of the interface boards (the ones that had their logo on the board) came to my brother and wanted to read him the riot act about not doing his job, when he noted that the label on the MAC address chip wasn't his. The vendor rep thankfully did the right thing and picked up the phone then and there and put a stop work and "you're fired" call into the contract vendor.

    It started out being a WTF, but somebody actually "fixed" the problem the proper way.

    We can only hope that others attempt to follow the example.

  • Ol' Bob (unregistered)

    Another example of "let the junior guy write the tests - it'll be good training". Feh.

    CAPTCHA: wisi The tests all executed perfectly, and wisi fast!

  • Benjamin Abbitt (unregistered) in reply to belzebub

    The code is testing the tests...

    The circle of test is maintained.

  • lucidfox (cs) in reply to snoofle
    snoofle:
    to this end, we use an in-memory db (bitronix), which lets you set up your test by creating a real table and populating it with good and bad data, then using it as your data source, and physically running the DAO, checking the results against expected values. Then, your teardown method drops the test table.

    This.

  • SomeCoder (unregistered)

    The real WTF is assuming that because it passed the unit tests, that's it good to go.

    I assume they caught this in integration testing but if they really just deployed it because it passed unit tests, THAT is the real WTF.

    At my work, we HAVE to stub out DAOs in a manning somewhat similar (though not exactly) to the OP's code because the machines that automate the unit tests cannot connect to any databases.

    In fact, unit tests are SUPPOSED to be self contained so that they do not require a database connection to function.

    The real WTF is the trend of the industry to assume that unit testing is the Silver Bullet we've been searching for for so long. It's not. Useful, yes, panacea, no.

  • Andres (unregistered) in reply to faoileag
    faoileag:
    Andres:
    You could have a valid but complicated SQL statement. Let's say it's running fine, but slow. You have a unit test verifying it. You go in and optimize the query. And then...

    If you have a mocked PreparedStatement that returns a mocked ResultSet based on the SQL and parameters passed in... your test is failing!! Even if the query is running correctly. You get the same failure if you insert a typo into the query, as if you optimized the query. Sure, you could then update the test, but that's a lot of work.

    If you unit test the sql statements used in your DAO, then yes, you will have to update the relevant unit tests once the statements change.

    If signatures or return values of your object's methods change, you will have to update you unit tests. Fullstop.

    "but that's a lot of work" is a lousy argument. "Did you test that release?" "No, too much work." "Ok, doesn't matter, it's just customer data we are loosing if anything goes wrong".

    Andres:
    And the unit test is still not giving you the information you need, which is whether the new SQL query is still returning valid data.
    What is so difficult to understand about unit tests? Unit tests verify the basic behaviour of an object, nothing more. getData() should return a List the same size as the ResultSet returned by executeQuery(). What data is in the list is irrelevant (for now).
    Andres:
    I think the only way to test DAO is against a real database that is kept in a known state.
    Than you should perhaps think again or dig a bit deeper into what unit tests are, and what not.

    Of course you should also test your DAO against a decent database. But not as part of a suite of unit tests.

    Here is a somewhat ideal testing scenario: Unit tests: on every submit Functional tests: if possible automated on a nightly build. Application-as-a-whole test: by your human tester as soon as he can test something. Say, pre-alpha. User acceptance test: if you work for a customer done by that customer once you are confident your software will pass the UAT.

    Well you got me, it's not really unit testing, it's integration testing. But it's being tested, normally as it's even developed (TDD). So if it's covered, I'm not too concerned if it's run with the unit test suite or the integration test suite. In most of my projects they run together.

    Yes, it is too much work updating my mock database system as needed by the DAO (mock PreparedStatement, ResultSet), for the value I would get out of it. Testing that given a ResultSet with 10 items, I get a List with 10 items, is not important for me if the String with the SQL has an error and would return an exception. I would rather check the SQL and the data extraction at once, since they belong to the same unit (the DAO). This is my opinion, but going crazy with mocking to unit test components like this, leads to loosing focus on getting the DAO to run against a real database.

    Here's how my testing looks:

    Unit Testing: Mostly Services and utility classes (for example ResultSetExtractor and RowMappers) with mocked dependencies. This runs after every commit. (White box) Integration Testing: Real Services and Real DAO against a pristine database with the latest schema. This runs after every commit. (Black box) Integration Testing: A different project that tests the full application. Runs periodically, depending on the project. Human testing as appropriate.

    Of course YMMV

  • chubertdev (cs)

    "Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." --Brian Kernighan

    The same is probably true of writing unit tests for your code.

  • mangobrain (unregistered) in reply to lucidfox
    lucidfox:
    snoofle:
    to this end, we use an in-memory db (bitronix), which lets you set up your test by creating a real table and populating it with good and bad data, then using it as your data source, and physically running the DAO, checking the results against expected values. Then, your teardown method drops the test table.

    This.

    Seconded.

    This is what I was getting at originally. No stub PreparedStatement will verify that the SQL is correct unless it actually attempts to execute that SQL. So either you need to move to integration testing against a real DB, or use a local file-backed/in-memory DB which still actually executes the SQL, but operates on known, canned data without requiring network access.

    faoileag, your suggestions are continually missing the point, which is that the SQL statements are a core part of the DAO's functionality, and they may be incorrect. You can't test the DAO by doing a string compare on "expected SQL statement" and returning "expected results" and then claim the DAO works, because you haven't actually verified that the SQL statement on a real database would return those results - you've just hard-coded the assumption into your test harness!

    (Sorry for over-using bold text, but my point is clearly not getting through, despite being understood by at least two others here.)

  • Mason Wheeler (unregistered) in reply to lol
    Comment held for moderation.
  • S (unregistered) in reply to faoileag
    faoileag:
    In cases where variables play a role (e.g. in the WHERE clause), you either compare by regex (more generic apporach) or you test against a few literal versions of the DAO's sql statement with the variable value hardcoded in.

    In most cases where the SQL varies based on variables, it's because the developer is embedding those variables into the SQL string, instead of passing them as parameters to the prepared statement, opening the door for an injection attack. As such, this should be regarded as a shooting offense...

  • S (unregistered) in reply to faoileag
    faoileag:
    Some typical test cases would be: - does getData() return a List the same size as the ResultSet delivered by my mock PreparedStatement? - does getData() return an empty List if ps.executeQuery() returns an empty ResultSet? - does the DAO throw an exception if con.prepareStatement(sql) or ps.executeQuery() throw one?

    Or for another one, null handling. ResultSet.getInt() (and getLong, getDouble, etc) returns a primitive rather than an object, and so returns 0 for a null column. If this is undesirable behaviour (and it usually is), you should be testing that the code has correctly distinguished between a real 0 and a null (via ResultSet.wasNull()).

  • dkf (cs) in reply to S
    S:
    In most cases where the SQL varies based on variables, it's because the developer is embedding those variables into the SQL string, instead of passing them as parameters to the prepared statement, opening the door for an injection attack. As such, this should be regarded as a shooting offense...
    Finally! A good use for the silver bullet…
  • Evan (unregistered) in reply to belzebub
    belzebub:
    Tests are testing the code. Good. But what's testing the tests? I demand tests written to test the tests!
    Actually there's a technique for this called "mutation testing." I don't think it's widely deployed (I think there are at least some problems with it in practice, I think part of the problem is I don't think there are free tools for C and C++), but it's a kind of neat idea.

    Basically what you do is produce lots of variants of your program ("mutants") by doing minor tweaks like replace "<" with "<=" or replace "1" with "0" or some higher-level things like replace the use of one concrete class with another. The assumption (and one problem with mutation testing is that this assumption doesn't always hold) is that the new program is (1) functionally different from the original, and (2) incorrect -- and thus there should be a test which detects that incorrectness. So you compile and test each mutant. If one of your tests passes on the baseline program but fails on the mutant, then that's good (you've "killed the mutant" -- I'm not making up that terminology), and if a mutant makes it through the test suite with everything passes than that indicates that your test suite has a hole in it.

  • Coyne (cs) in reply to chubertdev
    chubertdev:
    "Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." --Brian Kernighan

    The same is probably true of writing unit tests for your code.

    Add regression testing and it can go completely wonky. It's going to take me half a week to test a program after deleting functionality.

Leave a comment on “But the Tests Prove it Works Correctly!”

Log In or post as a guest

Replying to comment #:

« Return to Article