testing 2022-01-27 | Slack Archive

mauricio.szabo19:01:19

Yeah, I also don't like this specific test. I mean, I can see useful things for testing with a real DB (and in fact, I hate when tests mock the DB unless the DB is a side-effect on that specific test and don't affect in any way the result) but in this case is just wrong. To enumerate all the things I don't like about it (in a Thread)

mauricio.szabo19:01:19

Starting with (deftest insert-user!--test. I hate when people bind a specific function name to a test. It creates an illusion of "one function tested by one test" and that makes some tests incredibly brittle and flaky

➕ 1

mauricio.szabo19:01:42

_ (store/insert-user! user)
        res (store/find-by-email "")]
    (is (= (assoc user :id 1) res))))

Here I see multiple issues: 1. Implicit DB. Better would be to receive a connection, maybe one that is even tied to a transaction already running (and with Rollback set) so the test will be faster, more reliable, and we don't have to worry about tearing down; 2. Side-effect on the let. I mean, the find-by-email is not that big, so it could become part of the test (is (= ... (find-by-email...)) 3. The test assumes that what will come back from the DB is in the same format that what it is. This may not be the case at all, and may introduce resistance to refactoring the code

mauricio.szabo19:01:04

4. It's kinda internal state. I mean, it really doesn't matter that you have a function that inserts a user into a DB, right? What matters is that if that user was inserted at the right moment. So I would prefer an API test that did POST something, then GET something and see if there's a user with some of the data that you sent to it. It's almost the same speed as doing this test if you use the test adapters for Ring/Pedestal/etc;

mauricio.szabo19:01:33

5. it does test internal state. It's better to use something like nubank/matcher-combinators to check if the return matches some fields that you want to be the same (for example, e-mail) and others that you don't know what they should be (for example, password and ID)

mauricio.szabo19:01:24

#Also sent to the channel

In fact, this test induces an error in your code! Password should not be the same because you can't, ever, save a password in a DB in plain-text format!

💯 1

mauricio.szabo19:01:24

replied to a thread:Yeah, I also don't like this specific test. I mean, I can see useful things for testing with a real DB (and in fact, I hate when tests mock the DB _unless_ the DB is a side-effect on that specific test and don't affect in any way the result) but in this case is just wrong. To enumerate all the things I don't like about it (in a Thread)

In fact, this test induces an error in your code! Password should not be the same because you can't, ever, save a password in a DB in plain-text format!

💯 1

Colin P. Hill20:01:21

Ahaha wow that's a good catch

Colin P. Hill20:01:06

Although, conceivably, that's implemented as the concern of higher level code. Nothing should ever write to the password field without first salting and hashing, but this could be modeled as a logically distinct responsibility from plain IO.

mauricio.szabo21:01:39

I don't know if I do agree... I mean, in Clojure, the insert-user is just (jdbc/execute [<string-here> <params-there>]), so it makes sense to normalize things in this point - specially because by not doing so, you risk messaging other developers (including yourself in the future) that this function is completely fine to write users to DB, which is not in this case.

Colin P. Hill21:01:37

I'm not saying I advocate this, just that it's not a totally indefensible or unimaginable design. We're looking at one piece in isolation, and in looking at it we should consider that this concern might be addressed elsewhere.

👍 1

mauricio.szabo21:01:24

Yeah, I know. What I think is just that there should be no public or tested (specially not public and tested) functions that allow you to do the wrong thing 😄. It's obviously not that easy, that's for sure 🙂

2022-01-27

Channels