4 ms·
You should pull the PR you're reviewing down into your IDE and you should spend like, an hour reviewing it carefully and trying to break it. Have a mental check
by kmtrowbr 5y ago
You should pull the PR you're reviewing down into your IDE and you should spend like, an hour reviewing it carefully and trying to break it. Have a mental checklist: say they renamed a function ... is the file named the same as the function? Did they remember to rename the file? Did they delete parts of the code? Did they remember to delete everything related to that? Often the most challenging parts are negative: it's harder to remove properly than it is to add new things. Say they changed the validation on a model: is this going to work well with the production database? Do we need to write a migration to catch the database up to the new reality of the code? Automated tests are just automated warning flags that give you confidence that the codebase is basically hanging together. You still need to be very careful & thoughtful as you're making changes.
I like to actually reset the branch so I can see all the raw changes in my IDE, this helps me to think it through, in git you can do this via:
git pull feature-branch;
git checkout develop;
git checkout -b my_initials/feature-branch;
git merge feature-branch --squash --no-commit
- efficax 5y agoAn hour is way too much time. If you need that long the pr is too big and needs to be broken up into smaller parts
- QuercusMax 5y agoHow big are these changes? That sounds horrendously slow. If you don't trust your tests, write better tests. If you don't trust your migrations, write better tests.
- Dunedan 5y agoIn probably most cases the person who does the code changes does write tests for it as well. So as a reviewer without understanding what the code and the tests do, how do you want to assess if the tests actually do something useful?
- QuercusMax 5y agoIf your code and tests are so opaque that the reviewer can't understand them without physically running and modifying the code, then as a reviewer you should ask the author to make it clearer. Because otherwise that's how you get write-only code. Things like code style and documentation can make a big difference, and review time is when you can help enforce them and build team culture and knowledge.