7 ms·

Had exactly that bug in production, was using ruby on rails & active merchant, and some version change in ActiveMerchant switched from cents to dollars for one

by jacobn 4y ago

Had exactly that bug in production, was using ruby on rails & active merchant, and some version change in ActiveMerchant switched from cents to dollars for one of the integrations. Our test harness didn't catch it (weird combination of reasons, too long ago for me to remember the details) & it rolled out. Shortly thereafter I get an anxious customer call that we'd charged their debit card $2500.00 instead of $25.00 and they'd gotten an overdraft notice. At first I was incredulous ("how is that even possible!?"), then I remembered that we'd just version bumped ActiveMerchant. My endocrine response as I realized what must have happened was amazing to experience - the sinking feeling in my gut, hairs standing up, sweaty palms, dread, pupils dilating, and my internal video camera pulling back poltergeist-style in a brief out-of-body experience. Fun times. Live and learn.

Eleison23 4y ago
>internal video camera pulling back poltergeist-style That cinematic technique is called a "dolly zoom" and it's totally dramatic, if used correctly!
- cratermoon 4y ago
  Alfred Hitchcock famously first used it in his movie Vertigo, after one of his camera operators, Irmin Roberts, came up with the effect.
HappySquirrel2 4y ago
I made an account just to tell you that I was able to almost feel & experience what you 're describing just by reading your comment. There should be a market for this hah
mbell 4y ago
> Our test harness didn't catch it (weird combination of reasons, too long ago for me to remember the details) & it rolled out. This is why I'm pretty dogmatic about variable comparison in tests. This is dangerous and stuff like this has caused a lot bugs to slide though in my experience (and maybe ops): expect(account1.balance).to eq account2.balance This is safe and specifc: expect(account1.balance).to eq 2500 expect(account2.balance).to eq 2500 Unfortunately I've run into a lot of folks that take major issue with the later because of 'magic numbers' or some similar argument. In tests I want the values being checked to be be as specific as possible.
- hinkley 4y ago
  One of the tenets of unit testing, I don’t know if I derived this for myself or got it from someone, is that you get a lookup on the right side of the expectation or the left, but not both. There are too many yahoos out there writing impure functions or breaking pure ones that will mangle your fixture data. And the Sahara-DRY chuckleheads who see typing in 2500 twice (but somehow are okay with typing result.foo.bar.baz.omg.wtf.bbq twice) as some crime against humanity exacerbate things. Properly DAMP tests are stupid-simple to fix when the requirements change.
  - diarrhea 4y ago
    I get the intention, but a simple x = 2500 would satisfy both worlds.
- majormajor 4y ago
  Various things like "manual to change them" that make "magic numbers" bad in regular code make them good for testing (or at least less bad, a constant for that is still what I'd usually use, but a pretty specific constant, sometimes at the unit test level - shared ones get dicey). Agreed on the ease of having problems of using variables on both sides.
  - hinkley 4y ago
    One of the biggest ways that test code is not production code is that test code is only read by humans when the tests are failing. Whereas any time I'm working on a regular feature I am likely to be looking at log(n) lines of our codebase due to the logic that exists around the code I'm trying to write, and changing loglogn lines of existing code to make it work - if the architecture is good. Code that is write once read k < 10 times has very different lifecycle expectations than code that is constantly being work hardened.
- NortySpock 4y ago
  I fend off the "magic numbers" people with a variable with a name that describes what the magic number is for. var expectedAccountBalance = 2500 expect(account1.balance).to eq expectedAccountBalance expect(account1.balance).to eq account2.balance
dasil003 4y ago
I also had this problem—with the same stack—but the issue was quite subtle. It was due to an upgrade of the Money gem that changed the way "cents" were handled. In our case we had test coverage, including integration tests with VCR recordings to the payment gateway. But the problem was that the bug only affect Japanese Yen, and we did not cover every single currency.
tuatoru 4y ago
Reasonableness checks are nearly absent these days. Warning the user "that's a lot of money/a date far in the future/a large quantity.[1] Are you sure?" detracts, I suppose from the UX. Except it really doesn't. 1. "Reasonable" varies according to the particular situation (customer's order history, credit rating, the normal range of quantites for the product, etc.)
- brundolf 4y ago
  I think it's more that it's a game of whack-a-mole. There are an infinite number of possible scenarios you could warn about, and each one carries a small cost to implement, a small cost to maintain, and a risk of false-positives. Which ones are worth implementing can be hard to know ahead of time (implementing ones that have actually caused problems would be one strategy for narrowing it down, but the point remains that it's not as simple as "just check for all the unreasonable states")
hinkley 4y ago
Frankly, the ActiveMerchant team broke your trust. There's two things you can do with people you don't trust. Either cut them out of your life, or start verifying everything they tell you. I've dumped libraries with this sort of chaotic 'refactoring' (or not picked them because I could see they had made such poor initial choices that it was inevitable). I've also written unit, integration, or smoke tests for third party code with a high Precautionary Principle quotient, sometimes with its own separate build pipeline that takes green versions of our code to run them against latest of theirs. And for any code we got from a customer or business partner? This is practically a requirement for my sanity. They think that because money is changing hands they can do whatever they want and we just have to eat it. We don't have to eat anything. That doesn't catch the problem the moment it happens, but most times that's sufficient to catch it before anything hits production.

by jacobn 4y ago

Eleison23 4y ago

>internal video camera pulling back poltergeist-style That cinematic technique is called a "dolly zoom" and it's totally dramatic, if used correctly!

cratermoon 4y ago
Alfred Hitchcock famously first used it in his movie Vertigo, after one of his camera operators, Irmin Roberts, came up with the effect.

HappySquirrel2 4y ago

I made an account just to tell you that I was able to almost feel & experience what you 're describing just by reading your comment. There should be a market for this hah

mbell 4y ago

> Our test harness didn't catch it (weird combination of reasons, too long ago for me to remember the details) & it rolled out. This is why I'm pretty dogmatic about variable comparison in tests. This is dangerous and stuff like this has caused a lot bugs to slide though in my experience (and maybe ops): expect(account1.balance).to eq account2.balance This is safe and specifc: expect(account1.balance).to eq 2500 expect(account2.balance).to eq 2500 Unfortunately I've run into a lot of folks that take major issue with the later because of 'magic numbers' or some similar argument. In tests I want the values being checked to be be as specific as possible.

hinkley 4y ago
One of the tenets of unit testing, I don’t know if I derived this for myself or got it from someone, is that you get a lookup on the right side of the expectation or the left, but not both. There are too many yahoos out there writing impure functions or breaking pure ones that will mangle your fixture data. And the Sahara-DRY chuckleheads who see typing in 2500 twice (but somehow are okay with typing result.foo.bar.baz.omg.wtf.bbq twice) as some crime against humanity exacerbate things. Properly DAMP tests are stupid-simple to fix when the requirements change.
- diarrhea 4y ago
  I get the intention, but a simple x = 2500 would satisfy both worlds.
majormajor 4y ago
Various things like "manual to change them" that make "magic numbers" bad in regular code make them good for testing (or at least less bad, a constant for that is still what I'd usually use, but a pretty specific constant, sometimes at the unit test level - shared ones get dicey). Agreed on the ease of having problems of using variables on both sides.
- hinkley 4y ago
  One of the biggest ways that test code is not production code is that test code is only read by humans when the tests are failing. Whereas any time I'm working on a regular feature I am likely to be looking at log(n) lines of our codebase due to the logic that exists around the code I'm trying to write, and changing loglogn lines of existing code to make it work - if the architecture is good. Code that is write once read k < 10 times has very different lifecycle expectations than code that is constantly being work hardened.
NortySpock 4y ago
I fend off the "magic numbers" people with a variable with a name that describes what the magic number is for. var expectedAccountBalance = 2500 expect(account1.balance).to eq expectedAccountBalance expect(account1.balance).to eq account2.balance

dasil003 4y ago

I also had this problem—with the same stack—but the issue was quite subtle. It was due to an upgrade of the Money gem that changed the way "cents" were handled. In our case we had test coverage, including integration tests with VCR recordings to the payment gateway. But the problem was that the bug only affect Japanese Yen, and we did not cover every single currency.

tuatoru 4y ago

Reasonableness checks are nearly absent these days. Warning the user "that's a lot of money/a date far in the future/a large quantity.[1] Are you sure?" detracts, I suppose from the UX. Except it really doesn't. 1. "Reasonable" varies according to the particular situation (customer's order history, credit rating, the normal range of quantites for the product, etc.)

brundolf 4y ago
I think it's more that it's a game of whack-a-mole. There are an infinite number of possible scenarios you could warn about, and each one carries a small cost to implement, a small cost to maintain, and a risk of false-positives. Which ones are worth implementing can be hard to know ahead of time (implementing ones that have actually caused problems would be one strategy for narrowing it down, but the point remains that it's not as simple as "just check for all the unreasonable states")

hinkley 4y ago

Frankly, the ActiveMerchant team broke your trust. There's two things you can do with people you don't trust. Either cut them out of your life, or start verifying everything they tell you. I've dumped libraries with this sort of chaotic 'refactoring' (or not picked them because I could see they had made such poor initial choices that it was inevitable). I've also written unit, integration, or smoke tests for third party code with a high Precautionary Principle quotient, sometimes with its own separate build pipeline that takes green versions of our code to run them against latest of theirs. And for any code we got from a customer or business partner? This is practically a requirement for my sanity. They think that because money is changing hands they can do whatever they want and we just have to eat it. We don't have to eat anything. That doesn't catch the problem the moment it happens, but most times that's sufficient to catch it before anything hits production.