Hacker News Reader

Comment by icedchai | Hacker News Reader

I work with someone who regularly opens PRs for untested code. I'm talking stuff that hasn't even been run once: missing imports, undefined variables, etc... not bugs. I'm fine with bugs. I'm not fine with not testing your work in the most basic sense.

PR reviews don't mean throwing crap over the wall and hoping the reviewer figures it out. With this guy, there is so much back-and-forth hand holding, it would be simpler to close the PR and do it myself. But I don't...

View on HN

Sounds like something to raise with their manager. Or with this person, before even reading the PR, "Hey - just to check before I review this, does it work on your machine? Have you tested it?". If they say no, close the PR and tell them to do that before opening it. If they lie, call them on it. If they aren't learning, don't waste your time hiding this useless coworker's failures.

I have raised it in the past and will do so again. One of the problems is the manager has no software engineering experience and his view of "working norms" often go against mine.

Some of my complaints are fairly basic: test/review your own work before asking someone else to review it. This should be applicable regardless of industry.

time to go next?

I've been thinking about that for a while, for a variety of reasons. This is one of the smaller ones. Probably after the holidays.

It is applicable regardless of industry. If your manager isn’t technical, drag the nearest senior engineer into the conversation too.

As for the PR, don’t bother. “Hey this code you’ve submitted for code review doesn’t even compile. I’m your colleague, not a human compiler. Please don’t waste my time with this again” -> Close issue.

I'm 100% fine with opening a PR for "I just typed it" code...

with two caveats:

- a HUGE disclaimer at the top saying "DO NOT MERGE: not tested"

- also, you'll probably want to politely ask someone for a review, and be specific about what you're looking for

"Draft PRs" are fine for discussing topics or code or goals with a team mate.

It's all about getting feedback!

That's fine. What I'm talking about isn't that at all. This was a "this feature is done, here you go" PR.

Looping back to TFA: I've been trying to use this workflow more to get quality code written more quickly. I definitely have a habit of spending too much time polishing code - getting a draft/do-not-merge PR up with the core of my proposed changes up for review helps me avoid that, plus getting an earlier review helps with finding issues.

In addition to what the others have said, make sure you have a CI and don’t review the PR until that’s showing green. The CI should ideally include tests, linting according to a common code standard and other “grunt tasks” that are unnecessary in a PR review.

If you are in charge of the review process, simply start closing their PRs, list the reasons why, or better yet create a document you can reference establishing your guidelines. Don't back and forth; if they can't meet a simple set of guidelines, the PR is not ready for review.

Let their manager take issue with it. Fight it, bring it to their manager's manager, whatever. A company like that is not worth working for, so try to make it a company that is, or leave, but do yourself a favor and find a team you love.

> better yet create a document you can reference establishing your guidelines.

This. Remove as much ambiguity and grey area as possible.

> better yet create a document you can reference establishing your guidelines. How about working on a document that establishes the companies guidelines?

I can not imagine having to meet everyone's personal quality bar.

You don't have to meet everyone's personal quality bar, just the person who is in charge of reviewing your code. Write good code and you won't have a problem!

My expectations here were a bit lower, given the individual involved: write code that actually runs.

Ha! I think you might be asking for too much these days!

Isn't this what draft PRs, CI/CD and automated integration tests are for?

This sounds like what I do, except I open them as draft PRs in some functional state that I've run locally first in order to solicit feedback as early as possible asynchronously.

Most teams I've seen don't practice any sort of iterative development, despite claiming to do so.

Wouldn't a linter catch most of this?

You can also block commits with listing errors

You are not alone. It is not fun.

Reply on news.ycombinator.com