5 ms·

There was only one mistake that the article felt like giving a header to: "The cardinal mistake is inconsistency" The instinct to keep doing things the wrong w

by mrkeen 2y ago

There was only one mistake that the article felt like giving a header to: "The cardinal mistake is inconsistency" The instinct to keep doing things the wrong way because they were done the wrong way previously is strong enough across the industry without this article. I love to > take advantage of future improvements. However, newer and better ways of doing things are almost invariably inconsistent with the established way of doing things. They are dutifully rejected during code review. My current example of me being inconsistent with our current, large, established database: Every "unit test" we have hits an actual database (just like https://youtu.be/G08FxxwPjXE?t=2238 https://youtu.be/G08FxxwPjXE?t=2238). And I'm not having it. For the module I'm currently writing, I'm sticking the reads behind a goddamn interface so that I can have actual unit tests that will run without me spinning up and waiting for a database.

jrockway 2y ago
I like keeping things consistent even if the consistent way is "wrong". One thing that bugged me about the large codebase I most recently worked on is that we used a custom assert library for tests. The Go team says this about them: https://go.dev/wiki/TestComments#assert-libraries https://go.dev/wiki/TestComments#assert-libraries , and having learned Go at Google, I would never have been allowed to check in code like that. But this place wasn't Google and there were tens of thousands of lines of these tests, so I told new developers to keep doing things the "wrong" way. This didn't cause many problems, even if failing tests failing too soon is pretty annoying. Most of the time the tests pass, and the yes/no signal is valuable even if you can debug more by simply `t.Errorf(...)` and continuing. As for starting databases during tests, it's saved me a lot of trouble over the years. One time, we used sqlite for tests and Postgres for production. We had some code that inserted like `insert into foo (some_bool) values ('t')` and did a query like `select * from foo where some_bool='true'`. This query never matched rows in the tests, because t != true in SQLite, but t == true in Postgres. After that, I found it easier to just run the real database that's going to be used in production for tests. The only thing that behaves identically to production is the exact code you're running in production. Over here, I have code that uses a hermetic Postgres binary (and chain of shared libraries because Postgres hates static linking) that starts up a fresh Postgres instance for each test. It takes on the order of a millisecond to start up: https://github.com/jrockway/monorepo/blob/main/internal/testpostgres/testpostgres.go https://github.com/jrockway/monorepo/blob/main/internal/test.... The biggest problem I've had with using the "real" database in tests is low throughput because of fsync (which `perf` showed me when I finally looked into it). Fortunately, you can just disable fsync, and boy is it fast even with 64 tests running in parallel. One thing that's been slow in the past is applying 50 migrations to an empty database before every test. When you have one migration, it's fast, but it's one of those things that starts to slow down as your app gets big. My solution is to have a `go generate` type thing that applies the migrations to an empty database and pg_dumps resulting database to a file that you check in (and a test to make sure you remembered to do this). This has two benefits; one, tests just apply a single SQL file to create the test database, and two, you get a diff over the entire schema of your database for the code reviewer to look at during code reviews. I've found it incredibly useful (but don't do it for my personal projects because I've been lazy and it's not slow yet). Overall, my take on testing is that I like an integration test more than a unit test. I'd prefer people spend time on exercising a realistic small part of the codebase than to spend time on mocks and true isolation. This is where a lot of bugs lie. Of course, if you are writing some "smart" code and not just "glue" code, you're going to be writing a lot of unit tests. Neither replaces the other, but if you can spend 30 seconds writing a test that does actual database queries or 2 weeks mocking out the database so the test can be a unit test instead of an integration test, I'd tell you to just write the integration test. Then you know the real code works.
- jimbokun 2y ago
  I work on a service where a big percentage of the code is persisting to and reading from various stores, so unit tests have very limited value compared to integration tests.
  - jrockway 2y ago
    Yeah, if you can start up the actual things, then you know your code's going to work against the actual things. That's ultimately what we're aiming for.
    - mrkeen 2y ago
      That's not sufficient. The actual things are IO devices, and will sometimes fail and sometimes succeed. No judgement, just a fact of life. I code my tests such that my logic encounters successes, timeouts, exceptions, thread-cancellations, etc. All at unit-test speed. I can't trick an MSSQL deployment into returning me those results. It doesn't take 30 seconds to test what your system will do in 30 seconds.
  - mrkeen 2y ago
    Is this not a solved problem? Why do you need to write so much persistence logic?
    - jimbokun 2y ago
      I have no idea how to interpret your comment. Do you mean just throw an ORM library into your code and never give another thought to persistence issues? At scale, there will always be challenges with latency, through put, correctness, and cost of persisting and retrieving data that require considering the specifics of your persistence code. The service I’m describing handles abstracting these persistence concerns so other services can be more stateless and not deal with those issues.
Cthulhu_ 2y ago
If it's wrong then it needs to be fixed, obviously, but only if you fix it in a way that ensures consistency and doesn't break existing functionality. But the article doesn't mention wrong code per se, just different code. There's always multiple ways to solve a problem, stick to one for you and the 999 other developers' sakes. Your example is a good example; you call it a unit test, but if it hits a real database it's by definition an integration test. No mocked database will be as accurate as the real deal. It'll be good enough for unit tests (amortize / abstract away the database), but not for an integration test.
stickfigure 2y ago
You will find someday that you'd rather have tests that are comprehensive rather than tests that are fast. Especially when a significant portion of your program logic is in sql statements.
- mrkeen 2y ago
  Code which is unit testable is integration testable. Not the other way around. I test my units more thoroughly than integrations allow. Make the db return success, failure, timeout, cancellation, etc. One of my colleagues was trying to prevent a race condition towards the end of last year. He wanted the first write to succeed, and the second to be rejected. I suggested "INSERT IF NOT EXISTS". We agreed that it was the best approach but then he didn't put it in because the codebase doesn't typically use raw SQL.

by mrkeen 2y ago

jrockway 2y ago

I like keeping things consistent even if the consistent way is "wrong". One thing that bugged me about the large codebase I most recently worked on is that we used a custom assert library for tests. The Go team says this about them: https://go.dev/wiki/TestComments#assert-libraries https://go.dev/wiki/TestComments#assert-libraries , and having learned Go at Google, I would never have been allowed to check in code like that. But this place wasn't Google and there were tens of thousands of lines of these tests, so I told new developers to keep doing things the "wrong" way. This didn't cause many problems, even if failing tests failing too soon is pretty annoying. Most of the time the tests pass, and the yes/no signal is valuable even if you can debug more by simply `t.Errorf(...)` and continuing. As for starting databases during tests, it's saved me a lot of trouble over the years. One time, we used sqlite for tests and Postgres for production. We had some code that inserted like `insert into foo (some_bool) values ('t')` and did a query like `select * from foo where some_bool='true'`. This query never matched rows in the tests, because t != true in SQLite, but t == true in Postgres. After that, I found it easier to just run the real database that's going to be used in production for tests. The only thing that behaves identically to production is the exact code you're running in production. Over here, I have code that uses a hermetic Postgres binary (and chain of shared libraries because Postgres hates static linking) that starts up a fresh Postgres instance for each test. It takes on the order of a millisecond to start up: https://github.com/jrockway/monorepo/blob/main/internal/testpostgres/testpostgres.go https://github.com/jrockway/monorepo/blob/main/internal/test.... The biggest problem I've had with using the "real" database in tests is low throughput because of fsync (which `perf` showed me when I finally looked into it). Fortunately, you can just disable fsync, and boy is it fast even with 64 tests running in parallel. One thing that's been slow in the past is applying 50 migrations to an empty database before every test. When you have one migration, it's fast, but it's one of those things that starts to slow down as your app gets big. My solution is to have a `go generate` type thing that applies the migrations to an empty database and pg_dumps resulting database to a file that you check in (and a test to make sure you remembered to do this). This has two benefits; one, tests just apply a single SQL file to create the test database, and two, you get a diff over the entire schema of your database for the code reviewer to look at during code reviews. I've found it incredibly useful (but don't do it for my personal projects because I've been lazy and it's not slow yet). Overall, my take on testing is that I like an integration test more than a unit test. I'd prefer people spend time on exercising a realistic small part of the codebase than to spend time on mocks and true isolation. This is where a lot of bugs lie. Of course, if you are writing some "smart" code and not just "glue" code, you're going to be writing a lot of unit tests. Neither replaces the other, but if you can spend 30 seconds writing a test that does actual database queries or 2 weeks mocking out the database so the test can be a unit test instead of an integration test, I'd tell you to just write the integration test. Then you know the real code works.

jimbokun 2y ago
I work on a service where a big percentage of the code is persisting to and reading from various stores, so unit tests have very limited value compared to integration tests.
- jrockway 2y ago
  Yeah, if you can start up the actual things, then you know your code's going to work against the actual things. That's ultimately what we're aiming for.
  - mrkeen 2y ago
    That's not sufficient. The actual things are IO devices, and will sometimes fail and sometimes succeed. No judgement, just a fact of life. I code my tests such that my logic encounters successes, timeouts, exceptions, thread-cancellations, etc. All at unit-test speed. I can't trick an MSSQL deployment into returning me those results. It doesn't take 30 seconds to test what your system will do in 30 seconds.
- mrkeen 2y ago
  Is this not a solved problem? Why do you need to write so much persistence logic?
  - jimbokun 2y ago
    I have no idea how to interpret your comment. Do you mean just throw an ORM library into your code and never give another thought to persistence issues? At scale, there will always be challenges with latency, through put, correctness, and cost of persisting and retrieving data that require considering the specifics of your persistence code. The service I’m describing handles abstracting these persistence concerns so other services can be more stateless and not deal with those issues.

Cthulhu_ 2y ago

If it's wrong then it needs to be fixed, obviously, but only if you fix it in a way that ensures consistency and doesn't break existing functionality. But the article doesn't mention wrong code per se, just different code. There's always multiple ways to solve a problem, stick to one for you and the 999 other developers' sakes. Your example is a good example; you call it a unit test, but if it hits a real database it's by definition an integration test. No mocked database will be as accurate as the real deal. It'll be good enough for unit tests (amortize / abstract away the database), but not for an integration test.

stickfigure 2y ago

You will find someday that you'd rather have tests that are comprehensive rather than tests that are fast. Especially when a significant portion of your program logic is in sql statements.

mrkeen 2y ago
Code which is unit testable is integration testable. Not the other way around. I test my units more thoroughly than integrations allow. Make the db return success, failure, timeout, cancellation, etc. One of my colleagues was trying to prevent a race condition towards the end of last year. He wanted the first write to succeed, and the second to be rejected. I suggested "INSERT IF NOT EXISTS". We agreed that it was the best approach but then he didn't put it in because the codebase doesn't typically use raw SQL.