5 ms·

My team has a bike shedding sort of problem where a 100 loc PR will sometimes get scrutinized to hell, but a 3,000 loc PR will get LGTM'd by enough of the team

by donatj 2y ago

My team has a bike shedding sort of problem where a 100 loc PR will sometimes get scrutinized to hell, but a 3,000 loc PR will get LGTM'd by enough of the team to be merged before anyone that actually cares gets a chance to look at it. I would say the second half of that is the much bigger problem. People know who to ask to get a quick lgtm. I don't know what to do about it. I can't make people actually review. I've had unrealistic dreams like holding people responsible for things they approved that had bugs, but anything like that would be supremely unpopular. I could do something like requiring review from the team members that actually review, but they already feel overwhelmed by being the only ones that actually review. We tried setting soft limits on the size of PRs, but that comes with a lot of PR that are hard to review because the work is poorly divided and doesn't make sense in isolation.

simmschi 2y ago
Have you tried the code owners feature (assuming you're on Github). IMO a good approach is to have the actual code owners (i.e. the team responsible for a specific service or library) review the PR. If they think a shallow LGTM review of 3k LOC is enough, they can also deal with the bugs :-) If you don't have specific ownership in your code base I'd start there.
- donatj 2y ago
  This is all within a relatively small single team. As I said however, I could require review from specific people I know review but they're already at their wits end. Also having to explain why certain devs are required without it smelling like some sort of favoritism seems fraught.
  - Aeolun 2y ago
    Normally you promote those people to ‘senior’ and then say that a review by at least one senior is required?
Kinrany 2y ago
Anything you do to make people fix their 3000 loc PRs will be unpopular with those people
- sundbry 2y ago
  This, you should make soft and hard limits on PR sizes, providing exceptions only for automated changes or rare situations such as structural changes.
js8 2y ago
The reality is that you can't make people to care more about quality than their (yours) management cares about it. I would suggest discuss this with manager, and if he cares enough about quality, he can try to slowly coach the individuals to give better reviews. Or he can suggest that multiple experienced/reliable people also review the work.
lijok 2y ago
Speaks volumes to the maturity of our work that a 3000 LOC PR is seen not only as acceptable, but expected. Raising a planning change equivalent to a 3000 LOC PR in a civil engineering firm would get your assignment swiftly handed over to someone more competent.
- loa_in_ 2y ago
  LOC is a bad measure of literally anything. It's not related to productivity, efficiency, bug count, readability, etc. What it's most related to is the choice of language and libraries. For me, shunning big contributions just for being big just stifles my drive to contribute. If the solution does what it's supposed to, is valid, efficient and there's no obvious unnecessary duplication of codethen who cares if it's big. All valid contributions push the project forward.
  - sundbry 2y ago
    It is actually directly related to the time and complexity of code review. Just because LOC is an imperfect metric for productivity etc., does not mean it's a poor metric for risk, complexity, poor sequencing of changes, or other attributes.
- DontchaKnowit 2y ago
  How are you supposed to write new code without hitting a 3k loc pr?
  - Aeolun 2y ago
    Merge before you’ve written 3000 lines of code?
- Olreich 2y ago
  But the planning change to add a new wing to the building? That can’t be made smaller. What about if a new regulation is put in effect requiring every fire door to have a corresponding fire window next to it? That’s either 3000 little changes that need reviewing or one big change. One big change is often the correct choice. Knowing when it’s appropriate and when it’s not is the job of the engineer, the team, and the lead.
Leherenn 2y ago
Something helpful I have seen in this situation is to require the reviewer to test the changes for long PRs, and post proofs (a screenshot, logs, ...). Even with the best of intentions, it's easier to miss things in longer PRs, plus it gives more time for others if they want to have a look. It's not bulletproof of course, but at least it makes sure the basic functionality is working.
- rudasn 2y ago
  Agree. I've made it a habit the past few years to include before/after screenshots in my PRs and sometimes a screen capture of the UI/workflow changes. Also, screenshots in the issues pages for bugs or anything UI related. I find it makes it easier for people to see and understand the goal of your changes and helps them focus on the how in the code review. When others do that in their PRs I can jump straight into the tests files and get a better understanding of the what is proposed (or at least start the review process in a more substantial way - get the important stuff addressed early on).
Aeolun 2y ago
I like how when you ask people why they approved something obviously broken, they’ll say something like “Oh, I didn’t really look at it.” I mean, I do that too once in a while, but if bugs go through you own that.
camgunz 2y ago
The answer is to require a high percentage of test coverage. Then you skim through the code to get a general idea of what it does and how, then scrutinize the tests to verify they enforce that understanding. This does three things. First, it prevents people from dropping big chunks of code and asking their reviewers to make it work (if coverage is too low, it doesn't work). Second, the tests spare you the cognitive work of verifying the code does it's supposed to--especially edge and failure cases. Third, this frees you up to think about the bigger picture. Is this the right way to do this, should we be doing it at all, does this conflict with something else in the works, etc

by donatj 2y ago

simmschi 2y ago

Have you tried the code owners feature (assuming you're on Github). IMO a good approach is to have the actual code owners (i.e. the team responsible for a specific service or library) review the PR. If they think a shallow LGTM review of 3k LOC is enough, they can also deal with the bugs :-) If you don't have specific ownership in your code base I'd start there.

donatj 2y ago
This is all within a relatively small single team. As I said however, I could require review from specific people I know review but they're already at their wits end. Also having to explain why certain devs are required without it smelling like some sort of favoritism seems fraught.
- Aeolun 2y ago
  Normally you promote those people to ‘senior’ and then say that a review by at least one senior is required?

Kinrany 2y ago

Anything you do to make people fix their 3000 loc PRs will be unpopular with those people

sundbry 2y ago
This, you should make soft and hard limits on PR sizes, providing exceptions only for automated changes or rare situations such as structural changes.

js8 2y ago

The reality is that you can't make people to care more about quality than their (yours) management cares about it. I would suggest discuss this with manager, and if he cares enough about quality, he can try to slowly coach the individuals to give better reviews. Or he can suggest that multiple experienced/reliable people also review the work.

lijok 2y ago

Speaks volumes to the maturity of our work that a 3000 LOC PR is seen not only as acceptable, but expected. Raising a planning change equivalent to a 3000 LOC PR in a civil engineering firm would get your assignment swiftly handed over to someone more competent.

loa_in_ 2y ago
LOC is a bad measure of literally anything. It's not related to productivity, efficiency, bug count, readability, etc. What it's most related to is the choice of language and libraries. For me, shunning big contributions just for being big just stifles my drive to contribute. If the solution does what it's supposed to, is valid, efficient and there's no obvious unnecessary duplication of codethen who cares if it's big. All valid contributions push the project forward.
- sundbry 2y ago
  It is actually directly related to the time and complexity of code review. Just because LOC is an imperfect metric for productivity etc., does not mean it's a poor metric for risk, complexity, poor sequencing of changes, or other attributes.
DontchaKnowit 2y ago
How are you supposed to write new code without hitting a 3k loc pr?
- Aeolun 2y ago
  Merge before you’ve written 3000 lines of code?
Olreich 2y ago
But the planning change to add a new wing to the building? That can’t be made smaller. What about if a new regulation is put in effect requiring every fire door to have a corresponding fire window next to it? That’s either 3000 little changes that need reviewing or one big change. One big change is often the correct choice. Knowing when it’s appropriate and when it’s not is the job of the engineer, the team, and the lead.

Leherenn 2y ago

Something helpful I have seen in this situation is to require the reviewer to test the changes for long PRs, and post proofs (a screenshot, logs, ...). Even with the best of intentions, it's easier to miss things in longer PRs, plus it gives more time for others if they want to have a look. It's not bulletproof of course, but at least it makes sure the basic functionality is working.

rudasn 2y ago
Agree. I've made it a habit the past few years to include before/after screenshots in my PRs and sometimes a screen capture of the UI/workflow changes. Also, screenshots in the issues pages for bugs or anything UI related. I find it makes it easier for people to see and understand the goal of your changes and helps them focus on the how in the code review. When others do that in their PRs I can jump straight into the tests files and get a better understanding of the what is proposed (or at least start the review process in a more substantial way - get the important stuff addressed early on).

Aeolun 2y ago

I like how when you ask people why they approved something obviously broken, they’ll say something like “Oh, I didn’t really look at it.” I mean, I do that too once in a while, but if bugs go through you own that.

camgunz 2y ago

The answer is to require a high percentage of test coverage. Then you skim through the code to get a general idea of what it does and how, then scrutinize the tests to verify they enforce that understanding. This does three things. First, it prevents people from dropping big chunks of code and asking their reviewers to make it work (if coverage is too low, it doesn't work). Second, the tests spare you the cognitive work of verifying the code does it's supposed to--especially edge and failure cases. Third, this frees you up to think about the bigger picture. Is this the right way to do this, should we be doing it at all, does this conflict with something else in the works, etc