Hacker News new | ask | show | jobs
by fiddlerwoaroof 746 days ago
A couple times I've put a "if (now() > xxxxxx) fail()` in tests for this reason.
3 comments

I think this is the absolute worst way of dealing with the issue, and I truly hope everyone who sees this in a code-review has the sense to reject it.

By all means, have such a check as a compile-time error, but not as something that gets shipped to customers. The code you have now works, even if as a developer you'd like to refactor it. However, what if you ship this software to a paying customer, and then your company goes bust? At unknown point in the future, their perfectly good application suddenly stops working, without warning or obvious cause, and your company is no longer in business to simply change the time to a later one and recompile. Even if your company hasn't gone bust, why should the customer have to spend ages trying to figure out if they've done something wrong or it's the software that's at fault, before contacting your company, possibly being told to pay more money for an update even if they were otherwise happy with the old version they'd purchased.

Basically, this change is a liability for everybody working on the project and your customers. It'll soon get forgotten about, until you have people complaining that something just suddenly stopped working without warning. You might be able to "fix" it in seconds, but it can still require hours to triage the issue to discover what caused it before it ends up as your problem again, and probably significantly longer after your few seconds to fix it before the working version is back in the hands of the customer. For all that time, they've been inconvenienced just to save you the hassle of sticking a reminder in your own personal calendar or raising an issue in your bug tracking system or wherever else.

> By all means, have such a check as a compile-time error, but not as something that gets shipped to customers.

Tests don't get shipped to customers.

Ooops, I guess I missed the part where he said "in tests". That makes it a bit better, although when it triggers, it'll still need someone to triage why the test failed and either nudge the date up / delete the test / do the refactor. While that's going on, potentially you'll miss other failed tests because your overall build is red, or maybe you'll force someone to backout their change for no reason and stop other checking in that day. I actually worked at a place where if the build ever went red, all commits since the last successful build had to be backed out and nobody was allowed to check in again until the build was green again. This sounds drastic, but having lived with it for a couple of years, it grew on me - it stopped the repeated "Oh, that's just a one-line fix" that then broke something else and so on... Better to immediately revert the change and re-commit when you're sure everything will work. We also disabled and/or removed tests that will intermittently fail due to a fault in the test, and created an issue to write a better test if it was still relevant. The GP's test would have just deleted when it was discovered, rather than achieving its aim of getting someone else to do the refactor.

The best approach is still to do the refactor straight away if it's genuinely needed, or else create a task in your issue tracker to do it a later time if it's going to require resource that you don't have right now. Then PM can prioritise it accordingly, not have to react to an arbitrary timebomb placed in the code, which will invariably be discovered at a time that's not good for anyone.

Good in theory, but try that on a project with real customers and you’ll get a very simple “remove it and we’re done here” /shrug. Team’s processes and decision chain must have agreed to enforce this already, and that above is just a symptom
How did you get that past peer review?
Because people agreed that the failing test represented technical debt that needed to be cleaned up later.