Basically my team member submitted two PRs on Friday afternoon. First one is the PR that fixes a bug. Second PR adds tests that validate the bug is fixed. The reviewer wrote a comment on Friday night saying that PRs cannot be reviewed if there's no tests, so he suggests my team member to merge the PRs into one first and then re-submit for review. Then my team member said she'd separated into two PRs so it would be easier for the reviewer to review so the reviewer wouldn't get scarred when he sees the amount of files changed in the PR. Reviewer got a little offended by that comment and wrote back saying that the policy is to have all PRs have tests. Reviewer wrote back saying that it's not an enforced policy and asked if there's a personal reason behind him giving her a hard time. And this is still ongoing! If I were her, I would've just done what the reviewer had wanted! TC 160
These kind of reviewers are always pain
While I agree that the author of the prs could have been more flexible - in this case I think the reviewer is being more unreasonable. It is super easy to add a comment to the first pr with a link to the second pr saying the tests can be found here - hence marking as approved..
And why is it "hard" for the PR creator to just merge them into a single commit? Clearly the reviewer is no longer feeling that it will be a burden to review.
Exactly. Didn't she create both the PR at first by herself? Why is merging those PR (taken care by a few git commands) such a big task for her now?
My rule is unless thereās a reason not to, just do what people ask/suggest in a PR. Makes them feel good, and you donāt have to waste time arguing about silly things.
This
Some people like to be picky just to show they have some work. Sometimes I purposely create an obvious error that's already fixed in another pr. After the reviewer got baited, I submitted the fixed pr. Pretty fun when they did exactly what you predicted
The problem is if you merge the 1st PR and not the 2nd then you are practically deploying untested code
Just so basic, yet people are like, ohhhh the reviewer is at fault.
How easy is it to say merge the test pr first.
Reviewer should be explaining the reasoning behind the policy. If they arenāt, then this is a silly fight. If they are, then I expect the team member has a good reason to disagree. That said, it sounds like both have better things to do with their time on a weekend. There are a few reason why it should be merged btw. 1) a lot of CR tools verify code coverage. Itās easy to bypass by splitting a CR and having the second Cr with tests not covered everything. This leaves it up to the reviewer to manually check, which nobody wants to do 2) makes tracking easier. If there is a regression as a result of the issue, the first question will be āwhat kind of testing was doneā. Having them in a single commit makes it easier to find. 3) if I need to revert your change, now I need to revert two commits. It may not be obvious to Oncall that they need to do this. It can delay fixes if they only revert one and break the build, now they gotta debug that
> Reviewer should be explaining the reasoning behind the policy. Rational should be explained where the policy is written down. We have these policies so we don't have to continuously re-litigate this shit.
Such a lame excuse by the reviewee. Reviewers don't go over tests as such. They just check if code coverage is proper.
Giving "her" a hard time? Really? Enforcing quality and standards is now the equivalent of giving some girl a hard time???? This is what DEI has done to tech!!! Also you're assuming the reviewer got offended when all he said was to adhere to code review policy. While the girl actually asked (in the PR) if there's a personal reason behind giving her a hard time? Really? After the reviewer has clearly stated about an existing policy, she thinks she can play the victim card by alleging personal reasons?? What a B!!!!! Plus her tone is condescending. "So that the reviewer doesn't get scared by the amount of files changed'. Dear miss, no one will get scared but they sure will laugh at you for being such an idiot to create such a big PR. It will be thrown onto the shelf without reading a single line. That is exactly what the reviewer did. If she was really concerned about the size of the PR, why not break it into individually meregable functionality and raise a chain of PR with proper testcases? God!!!!!
Same, +1
Your anger about this seems to be stemming from the fact that the reviewee is a woman?? If not I donāt understand why bring gender into the discussion - and why would you assume she was hired to meet dei criteria? I share your frustrations with having any special hiring category based on gender - but your response seems needlessly sexist .
So both had a bad weekend to protect their egos. Cool š¤£
I'd probably just combine them. It's not worth the fight. Is tests with PRs the practice the team agreed on?
Just admit when youāre wrong