[mbell]

> 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]

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]

I get the intention, but a simple x = 2500 would satisfy both worlds.

[NortySpock]

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

[jsrcout]

That's my approach as well. My idea is I can change the numbers, and it still better come out right. It forces you to think through (and code) things more clearly. It's too easy to write a test with hard-coded numbers that "just happens" to come out right.

[tharkun__]

That's a good enough (initial) workaround if you're in an environment where you're in the minority (or alone) w/ your opinion and you still want to do the right thing personally.

I would advise you to try and convince your peers though and teach them the better way because I suspect that the people that you're fending off would not do what you did but rather just go w/ the one

    expect(account1.balance).to eq account2.balance

Now while parts of the code base do the right thing (in a slightly long winded way), the rest of the code base written by these other people is still using bad tests.

[ilyt]

as long as there is

expect(account1.balance) eq 2500 before that isn't really a big problem.

Also "how to do it" is kinda different topic to "how to convince peers that's the right way"

[majormajor]

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]

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.

[jiveturkey]

thanks. i do the same and consistently have to push back on reviewers (why don't they learn?) that the hardcoded number is there for a reason

[hinkley]

What the complainers often like to do is hoist variable declarations out of the local scope. So to them having a test suite that uses 1500 in four places is wrong, and up to that point they are perfectly right.

The trick with the constants is that if they are declared and used in the same test scope, then the data is a black box. Nobody else 'sees' it, nobody interacts with it. The only time that's not true is when there's false sharing between unit tests and those tests are fundamentally broken. In that case the magic number is not the problem, it's the forcing function that makes you fix your broken shit.