[pclmulqdq]

There's a culture of "I won't approve this unless it does something for me" at Google. So now changing the text on a button comes with 2 minor refactors, 10 obvious-but-ignored bugfixes, and 5 experiments that it is actually better.

[moregrist]

While this sounds pretty frustrating, there is at least a small upside: at least you get to the obvious-but-ignored bugfixes.

Most smaller places don’t have the bandwidth and many larger ones don’t have the desire.

I’m not sure if that makes up for bugs potentially introduced in the refactors, though.

[dietr1ch]

Well, when the owner asks for a whole test suite that didn't exist to get a fix in, what most likely happens is that you just wasted your time in a draft CL that will get lost.

[specialist]

Do you mean the relevant code area(s) didn't have (sufficient) tests? You're being asked to backfill those missing tests in addition to your fix?

[dietr1ch]

Yes. I've experienced pushback from obvious fixes with requests to formally test their code for the first time.

All because it may break someone. Even when I presented a real defect based on docs/comments and fixed it. You'd think that if they truly cared about breakages they'd already have some tests for it from where I can easily start.

[vkou]

> All because it may break someone. Even when I presented a real defect based on docs/comments and fixed it.

It's great that you found a bug and fixed it.

The problem is, how do you know that there are no other regressions?

Code is a liability. Once you check it in, the team that owns it is responsible for it. Untested code is a liability of unknown scope. It's quite understandable why they don't want to accept someone's contributions, when the contributor isn't the one who will ultimately be dealing with any of the consequences. If you think they are being mean and lazy, imagine if the tables were reversed.

I don't accept puppies or elephants as gifts for similar reasons.

It's unfortunate that existing test coverage sucks. In this case, the best way forward should be for the team in question to improve coverage, and for you to then submit your fix + a test for it. And if they don't have budget to do this, then that sucks, but that's their call to make, and that's a signal that the project in question is abandonware.

And it's fine for a large company to have a bunch of abandonware. If it works, and produces value, the optimal amount of ongoing development effort to invest into a piece of software may, depending on the circumstances, be near-zero.

[__turbobrew__]

I don’t think that is necessarily unreasonable. The team may have the same constraints on themselves in that they wouldn’t touch the code either until tests are written.

[specialist]

Thanks for clarifying. Sympathies.

I've been on every side of this conundrum. Not being allowed to improve/fix stuff is super frustrating, very demoralizing.

I can quickly recall 3 separate projects where we weren't allowed to fix stuff on new products that hadn't shipped yet (not even beta). (Oops, just thought of 2 and 1/2 more projects.)

It's hard to not treat that as beligerent, spiteful, gatekeeping, control issues. For me, half the time it absolutely was.

I've also managed legacy code bases that we dare not accidentally breathe on. Somewhere between archaeology and necromancy.

For me, when I'm in charge, project (engineering) management comes down to mitigating risk, usually by driving down the cost of change. I'm sure I'm preaching to the choir.

Alas, it's been a long time since I've been on a project that had anything resembling proper PMI, QA, Test, etc. So there's really no process or strategy to mitigate risks.

IIRC, the contemporary deliberate rejection of process is called something like the "Argyle" Methodology. Or maybe it's "Agile". Or "Argh-gargle". I don't quite remember. I can dig up those books and links, if anyone is pervously curious.

Well. Good luck.

[vkou]

They aren't asking for you to write tests because 'it benefits them', they are asking you to write tests because as a professional engineer, you should write tests, and not just yolo it.

Look, sometimes you may have good reasons for why a test is impractical. You are allowed to push back, or look for a different reviewer. There's a hundred thousand people in the firm, you should be able to find one or two that will let you submit literally anything that compiles.

But most of the time, the reviewer is giving you advice that you should take.

[pclmulqdq]

If you are turning a button to a slightly different shade of blue and it's not a button you own, the owner of the button should not be asking you to write tests for the button.

[vkou]

If the case is as simple as you describe, surely there's more than one owner of the code that can approve this, if one guy is being unreasonable.

If there is actually only one owner that can approve changes to the package, there's something really weird and unusual about that project setup, or it's someone's internal hobby project that they wrote five years ago and semi-maintain, in which case, I have to wonder why you submitting one-liner changes to it is all that important.

We're all adults, we all work together, we can all work this out. If someone absolutely insists on being an asshole, escalate. It's why you have a manager, and why they have a manager.

My experience is that very few people are unreasonable assholes.

There's always plenty of organizational, vision, strategy, and execution problem in any billion-dollar company, but 'people are unreasonable in code reviews' is not one I'd put in the top 10. It might be something that ruins your day once or twice, but that doesn't make it systemic.

[dietr1ch]

> If someone absolutely insists on being an asshole, escalate.

That's doubling down on time spent on contributing back. It's usually cheaper to workaround the issue once you notice it'll be way harder than it should be (not hard at all).

[ncruces]

It's as good an opportunity as any to improve things.

They're acting as selfish demanding you do something for them, as you are for refusing.