3 ms·

There's a huge difference between "zero testing" and "we didn't have this particular test case covered because the state space is massive". Lets say you even ha

by kortex 5y ago

There's a huge difference between "zero testing" and "we didn't have this particular test case covered because the state space is massive". Lets say you even have "100% code coverage", do you really have a unique case for each possible condition in that query? Often times chaining operators like TFA give "deceptive" coverage results because they mark that whole code path as covered, without verifying the state space of the operands is covered fully. You can sometimes "cheat" coverage e.g. by using ternary operator assignment in place of if/else (often by mistake).

brian_cloutier 5y ago
In the general case you are right, but in this specific case... this test case should have been covered. Even a rudimentary integration test would have caught this bug. It sounds like the user story is: - A user runs a query - The user does not have any queries left in their plan - They are billed. - When the user runs the query again they are not billed a second time. I'm struggling to imagine how the test would have failed to catch this. Maybe it was unit tested but with a database containing just one user? Maybe they got very unlucky and the correct user was randomly chosen?
- kortex 5y ago
  Yeah, they might have just had unit tests with mocked db, or there wasn't enough combinatorial variety. It's insufficient to test the lack of double billing - you have to specifically induce a race condition, assert the race occurred, and that the interlock prevented double spend. It's tricky. Personally I'd try to engineer around race condition entirely, like some scheme with tokens and pagination. I agree, this sort of thing seems like it should be extra-well covered, but, y'know, move fast and double charge folks.
- lucumo 5y ago
  You're demonstrating the opposite point perfectly. That test scenario would NOT have catched the issue. The problem wasn't that requesters that should've gotten billed didn't. The problem wasn't even that requesters that shouldn't have gotten billed did.[1] The problem was that clients OTHER than the requester got billed. You can, of course, write test cases that check your entire database for unintended state changes, but I struggle to find that a reasonable amount of effort. Especially since you'd have to do that for all code paths. That will very quickly cost a large multiple of the 73k this bug caused. The adequate monitoring and quick response they did here is probably a very good trade-off. Like it or not, production is ALWAYS your last test. Issues are less costly if you realise that, than if you don't. [1] Though for this specific bug, that scenario would've failed too, and might've triggered an extra look at the code.
  - brian_cloutier 5y ago
    > You can, of course, write test cases that check your entire database for unintended state changes, but I struggle to find that a reasonable amount of effort. I think you're overthinking this. I'm not suggesting they should have looked for any unintentional state changes, we both agree that is overkill (until you start doing FP). > Though for this specific bug, that scenario would've failed too, and might've triggered an extra look at the code. Yes, exactly. For this specific bug even the simplest test would have failed, which would have caused someone to take a look at what was happening. You are correct that if the bug had been more complicated, such as causing both the proper user AND an additional random user to be charged, then it's unlikely a reasonable level of testing would have caught it. > The adequate monitoring and quick response they did here is probably a very good trade-off. We agree that there is a trade-off here, and if sacrificing some correctness is what it takes to win you a much higher velocity then they probably made the correct trade off; nobody died as a result of this bug. But... surely you see there are some cheap steps they could have taken which would have caught this bug? Not all bugs, but this specific bug. - Write integration tests for important behaviors, such as charging users! - Make sure those integration tests run in an environment which closely simulates production.
deleted 5y ago
[deleted]
Retric 5y ago
> update your dependencies and ship it based on version numbers alone The above is zero testing. Update your dependencies and ship it based on version numbers and integration tests is different. In that case as you suggested test coverage may easily have missed something, but there’s moving fast and there’s moving blindly and the second is just wasteful.

by kortex 5y ago

brian_cloutier 5y ago

In the general case you are right, but in this specific case... this test case should have been covered. Even a rudimentary integration test would have caught this bug. It sounds like the user story is: - A user runs a query - The user does not have any queries left in their plan - They are billed. - When the user runs the query again they are not billed a second time. I'm struggling to imagine how the test would have failed to catch this. Maybe it was unit tested but with a database containing just one user? Maybe they got very unlucky and the correct user was randomly chosen?

kortex 5y ago
Yeah, they might have just had unit tests with mocked db, or there wasn't enough combinatorial variety. It's insufficient to test the lack of double billing - you have to specifically induce a race condition, assert the race occurred, and that the interlock prevented double spend. It's tricky. Personally I'd try to engineer around race condition entirely, like some scheme with tokens and pagination. I agree, this sort of thing seems like it should be extra-well covered, but, y'know, move fast and double charge folks.
lucumo 5y ago
You're demonstrating the opposite point perfectly. That test scenario would NOT have catched the issue. The problem wasn't that requesters that should've gotten billed didn't. The problem wasn't even that requesters that shouldn't have gotten billed did.[1] The problem was that clients OTHER than the requester got billed. You can, of course, write test cases that check your entire database for unintended state changes, but I struggle to find that a reasonable amount of effort. Especially since you'd have to do that for all code paths. That will very quickly cost a large multiple of the 73k this bug caused. The adequate monitoring and quick response they did here is probably a very good trade-off. Like it or not, production is ALWAYS your last test. Issues are less costly if you realise that, than if you don't. [1] Though for this specific bug, that scenario would've failed too, and might've triggered an extra look at the code.
- brian_cloutier 5y ago
  > You can, of course, write test cases that check your entire database for unintended state changes, but I struggle to find that a reasonable amount of effort. I think you're overthinking this. I'm not suggesting they should have looked for any unintentional state changes, we both agree that is overkill (until you start doing FP). > Though for this specific bug, that scenario would've failed too, and might've triggered an extra look at the code. Yes, exactly. For this specific bug even the simplest test would have failed, which would have caused someone to take a look at what was happening. You are correct that if the bug had been more complicated, such as causing both the proper user AND an additional random user to be charged, then it's unlikely a reasonable level of testing would have caught it. > The adequate monitoring and quick response they did here is probably a very good trade-off. We agree that there is a trade-off here, and if sacrificing some correctness is what it takes to win you a much higher velocity then they probably made the correct trade off; nobody died as a result of this bug. But... surely you see there are some cheap steps they could have taken which would have caught this bug? Not all bugs, but this specific bug. - Write integration tests for important behaviors, such as charging users! - Make sure those integration tests run in an environment which closely simulates production.

deleted 5y ago

[deleted]

Retric 5y ago

> update your dependencies and ship it based on version numbers alone The above is zero testing. Update your dependencies and ship it based on version numbers and integration tests is different. In that case as you suggested test coverage may easily have missed something, but there’s moving fast and there’s moving blindly and the second is just wasteful.