I understand the desire to make larger patches, but how do you effectively manage them in the review process? For super large commits in the past, I’ve had other engineers hop on a screen share to review the diff together and answer questions, but it feels inefficient.
how do you effectively manage them in the review process
The overriding principle is "no surprises at final review time."
For a big impactful change discuss the work in progress beforehand (specifically: architecture stuff, database changes, impact this will have on other code) with them as much as possible before final review time. There's no other sane way to do it.
Make sure you're all in agreement on the approach you're taking or, at least, make sure they're aware of it. Also a good way to (hopefully) make sure you're not going to conflict with somebody else's work!
Dumping a big commit full of surprises on reviewers is AT BEST poor practice and is at worst kind of shady and a sign you don't want to collab, don't respect your coworkers and/or or are hiding something.
I definitely prefer in avoiding surprises. In this case the review was upgrading and migrating from Vue 2 to Vue 3 and, while the rest of the team was in the loop and aware of coming changes, the change set itself was massive. I definitely would do it differently the next time around, and this is an edge case. I will say that that position saw several “larger than any reasonable person should submit” change sets, glad it’s behind me.
Y'all are very thorough. Just make sure the function has tests and won't blow up prod. You needn't waste forever reviewing. It's code. It can be iterated upon.
In this particular case, we're talking some tricky code around array handling in the JS implementation of a major browser. It's pretty much the archetypical location for a VM escape bug. That's most definitely not something you want to be cavalier about.
In general, the large, sprawling diffs we're talking about here, the sort that actually justify commits of this size, are almost always going to also be the sort that justify more closer scrutiny at review time.
No. In any actually complex piece of code even adding significant amounts testing over new code is not going to cover every possible code path.
It's also immensely difficult to write tests to find errors in code you're writing - most patches I see with "extensive tests" test all the correct code paths, and _maybe_ a few error cases. It's a very easy trap to get yourself into.
The purpose of review is not to catch obvious issues - though it hopefully will - but the subtle ones.
I’ve had both thorough and cursory reviews of changes like this.
It makes no difference for finding actual bugs.
Reviews are great for spreading knowledge about the change and making sure the style is dialed in. Reviews aren’t great for ensuring test coverage or finding security bugs.
Test coverage should be ensured by your coverage tools in the CI. (At least basic test coverage.)
You are right that reviews aren't necessarily there to catch bugs. Reviews are there to tell you to write code that's simple and understandable. If your coworkers can't understand what's going on, how could you expect anyone else to do so in a year or so?
Coverage tools only tell you have tests that hit each line, but most serious bugs aren't the "you didn't hit one line", they're the result of specific interactions between multiple paths. e.g. (note this is a super simple and trivial example)
int size = 0;
char *buffer;
if (a) {
buffer = new char[1];
size = 1;
} else if (b) {
buffer = new char[2];
size = 2;
}
...
if (b) {
buffer[1]++;
}
...
Coverage tests will happily show that every line is hit, without tests necessarily hitting the buffer overflow if an and b are both true.
Indeed. I always push my teams into not hunting for spelling mistakes. Instead look for things that will be hard to fix in the future ('did you realise that this bootloader config stops us doing field updates??') or where the overall direction is wrong ('Not sure about os.system('grep') for this XML - why can't we just use the xml.etree here?').
I worked for a while with Henrik Frystyk Nielsen, who worked on the early HTTP spec and httpd. We would give him a hard time for this — among other things.
I know that Phillip Hallam-Baker probably first used that spelling in a document
This is code running on client machines which you have no control over. Once it's out there good luck getting everyone to upgrade to your latest version because "hey, we didn't want this one to behave like that. oops". Especially when you're google and don't want certain freedoms in your software.
You make sure that more than one person is aware of the changes and design, and have all people frequently discuss the progress before the mega patch hits PR.
Then the mega patch PR is " one last look ", and not thousands of changes storming the gates of your sanity.
Yeah its best to go over something like this is a meeting/zoom or a series of meetings. And most likely not every single line will be gone over, at some point the engineer writing this code is likely pretty senior and shouldn't need that many eyes on their boilerplate, and its the nasty stuff that really needs talking about and focusing on.
Code review doesn't magically catch all bugs even with tiny PRs. At best it's a basic sanity check. In fact I'd say it's more of a design sanity check than a bug catching one.
To prevent bugs you need tests, fuzzing, static analysis, static & strong type checking, etc.
