Hacker News Reader

Comment by RickJWagner | Hacker News Reader

It sounds pretty fancy. I'd want a very thorough set of unit tests before trusting code to delete code, though.

Piranha co-author here. I'd say you trust it a bit more than we do, then ;) As the paper and blog post mention, we use Piranha to create diffs/PRs against the original author of the code. Developers are expected to code-review the changes before they are ever landed. As used within Uber, Piranha won't ever auto-land code deletions.

That still saves the time of: a) remembering that the flag is stale and must be removed, b) actually removing the code and putting the diff up. Piranha diffs for a single flag are also generally small enough to fully read them during code review.

We do have comprehensive unit tests for both the Piranha tool and the target codebase(s), and the majority (65%) of diffs generated by Piranha are landed without changes (and the most common change in those that are changed is to delete extra lines that Piranha couldn't prove as stale). Thus far we haven't had an outage caused by a Piranha deletion, but certainly there have been incorrect diffs generated and caught either by CI or manual reviewers, requiring us to update the tool. We would not recommend landing diffs generated by Piranha - or other stale code removal tools - to master without any reviews right now :)

Follow up question: In general, when a diff is generated, is the standard process to just do a manual code-review and CI, or do you also test it manually to check for unexpected side-effects?

Manual code review and CI only. In our experience of deleting more than 2.5K flags, testing would have helped in one case but then the code was not tested well when the flag was introduced.

Just as an extra note: there are manual testing steps (and automated end-to-end test, and internal alpha test, etc), independent of whether the diff was Piranha- or developer-authored, but all those happen for the continuous delivery internal version of the app, which is after the diff has been landed to master, but before a release is sent to app stores. We would count an issue discovered there as an outage having made past "our tests", even though it could well be caught before it gets to any external users.

Reply on news.ycombinator.com