Hello again, I’m in a situation where the one the senior devs on my team just isn’t following best practices we laid out in our internal documentation, nor the generally agreed best practices for react; his code works mind you, but as a a team working on a client piece I’m not super comfortable with something so fragile being passed to the client.
He also doesn’t like unit testing and only includes minimal smoke tests, often times he writes his components in ways that will break existing unit tests (there is a caveat that one of the components which is breaking is super fragile; he also led the creation of that one.) But then leaves me to fix it during PR approval.
It’s weird because I literally went through most of the same training in company with him on best practices and TDD, but he just seems to ignore it.
I’m not super comfortable approving his work, but its functional and I don’t want to hold up sprints,but I’m keenly aware that it could make things really messy whenbwe leave and the client begins to handle it on their own.
What are y’alls thoughts on this, is this sort of thing common?
breaks tests
leaves me to fix them during approval
I’m sorry, what? If he broke it, he fixes it. There should be guard rails that prevent him from merging his code until all the tests pass, and you as a reviewer should refuse to even start a code review unless the build is green.
Exactly, don’t even strat looking at a PR that doesn’t pass the CI pipeline
Im quite happy to help fix the tests with someone that is willing. This kinda guy, not so much
Yup. Nothing wrong with pushing up a draft PR and asking for feedback; but definitely need to be an active participant in fixing the issues, not just expect somebody else to do your work for you.
That does lead to some sticky inter-personal situations though. Like there’s people on my team that I trust enough to just rubber-stamp a PR that looks good but doesn’t have test coverage etc. Can generally trust those people will let me know if the failing tests uncover some substantial work that needs to be re-reviewed.
There’s other people I don’t trust and will insist their build passes before I review it. Once that person notices they’re being held to a different standard, it can be difficult (but necessary) to have a conversation about what they need to change in order to earn that trust.
I think this is far more common than one would hope. There are many senior developers out there who got their experience in a different time, when test coverage wasn’t important in many businesses. Writing test code is hard and it might be that your teammate simply don’t know how to do it.
If the tests aren’t there at approval time, they will never be there. I think it is perfectly fine to block approval, especially since you all agreed on it.Yes, it’s common. No, it shouldn’t be tolerated. Your boss/tech lead/whatever should be involved. Here is what should be done ideally:
- not following best practices: you MUST implement merge requests (GitLab, GitHub, etc.) and his code shouldn’t be approved which means that his code won’t ever be merged in a shitty state. Force 1 or 2 approvals for each MR, and it should not be possible to merge an MR if it has open comments. The boss should ask every day “why is your code not merged yet?” and he’ll have to explain why people don’t approve his shitty code.
- shitty unit-tests: same thing, the boss should show him how to do this, and the MR shouldn’t be approved.
- breaking unit-tests: it’s the job of the CI to literally block MRs that break unit-tests (whether it’s code coverage or unit-tests).
- leaves me to fix it during PR approval: NO, it’s HIS merge request, not yours.
To sum it up: devs must not approve his MRs, the CI must block MRs that break tests.
Last point is SO painful… I have a coworker that writes so much shitty code OR it straight up doesn’t work… he once submitted a PR that didn’t work when used! Like, I just started the thing and it was utterly broken - both the implementation and the design.
More so, some of his PRs are a giant nightmare of over engineered crap that he, at some point, doesn’t even understand.
Worst of all, he gets angry at me for pointing out that either they don’t work or they are a shitty, complex, mess
Honestly, at some point I started approving his PR and calling it a day; oh we don’t have tests cause reasons, I tried to use TS too but since my boss finds it too complicated we are not using it again for new projects… funny
This thread makes me so grateful for working with competent people.
just yesterday I had to drop a lot of his commits cause they broke some core functionality lol
It shouldn’t be up to another engineer to fix their PRs. They wrote the code, they are responsible for making sure it is in a state to merge. If it’s not, request changes and move on to your work.
Yeah, we comment in places where we see issues, leave tasks, and just mark the PR as “needs work.” I ain’t touching code in a branch that’s not owned by me.
Soooo much. It’s the biggest of red flags to have to fix other’s reviews.
Maybe take it up with managment. Those kind of profile are a hinder for every other devs that are working with them
Doesn’t sound like a senior dev to me. Sounds like someone who thinks they are.
Yes, well, seniority tends to be defined quite differently by management – compared to how other devs would define it. A senior to them is a person with a certain experience (at least 3-5 years), who has worked on at least a couple of complex projects (no telling exactly what they did there), shows a “can do” attitude, has good feedback from teammates, and last but not least delivers stuff on time.
Notice how quality of code doesn’t come into it at any point.
Management doesn’t know which code is “quality”, it’s all voodoo to them anyway. A pleasant team member who sounds like they know what they’re doing and delivers working stuff is all they need to see.
Quality of code needs to be defined and enforced on a project-by-project basis (definition of ready, definition of done). If they aren’t defined and/or enforced, but delivery still happens on time, it will be hard for a junior to demonstrate a problem. Some experienced managers will recognize it as a problem in the making by accumulating technical debt, some won’t (or don’t care).
I would suggest that OP explains the technical debt in impartial terms during sprint review and wash their hands of it. Confronting the other dev directly usually doesn’t work well, especially if it’s done remotely.
It’s not called ‘PR Approval’ it’s called PR Review for a reason. Developer should fix broken test especially if there’s super fragile stuff involved ( and that should be fixed asap).
Yup. Just add a comment that says “add tests for this” on lines of code that needs it in the review. If your dev ends up taking a couple of weeks to finish it, so be it.
It’s easy to become a 10x developer when you skip 90% of chores and let other devs doing them for you. If you keep enabling them, they’ll become 100x developer soon enough.
Very common.
Don’t feel pressured to approve anything you don’t want to, but still be chill. It’s just work after all. (This duality takes years to figure out, but if you can, you’ll be very valuable)
Get the PM involved. Bring it up in retro and stand up.
Examples.
“I don’t feel this is PR is up to our company standards. Here’s a link to the document. Specifically tests are breaking, coverage is reduced, and your using global variables. If you need help with quality we can code pair next sprint or if I finish my tasks early. Let me know”
“Just a reminder that we have 3 PRs with needs work sitting in the queue. If you’re not able to finish them before the end of the sprint, let the scrum master/PM know in case it’s a high priority”
“We’ve all signed off on a standards guideline, and lots of PRs are falling short. Either we need more training time each sprint to reach it, or were going to have to officially reduce our standards. Let me know which one the CTO prefers”
Clear management concern. Good luck! Remember it’s only a job
He causes tests to fail and leaves you to fix them?This sounds insane to me. No one should have to fix another engineers problems no matter their level.
That’s definitely not senior dev behaviour, was he also involved in defining the best practices? If so he should 100% be following them as an example to other devs, and if he has an issue with individual parts (which is fine) he should discuss with the team about changing them.
Aversion to tests is also a really concerning problem I would say. And you should not have to fix his code in PR, it’s his code!! I’m afraid I don’t have any answers for you but you are right to find this unusual!
Others have given excellent advices. I’ll approach it from management point of view:
-
If there’s management oversight, such as tech lead/engineering manager, talk to them. Don’t make any accusation. Approach it from the direction of you feeling uncomfortable with how the team is working. They will know how to solve the issue. However, any tech lead/engineering manager should have already dectected the problem and at a minimum acknowledge the issue.
-
If there’s no tech management oversight, I’d suggest you approach the senior engineer directly. I’d want to emphasize here that it has to be tech management. Non tech management won’t understand the problem and they won’t be able to solve the problem. Sometimes the senior engineer maybe under pressure to deliver and there’s nobody to split the tasks to other team members. I did this a few times in my career before I developed my skill to lead a team.
-
If it’s neither because the senior is under pressure to deliver, nor there’s management oversight, your next best bet is to seek consultantion with another senior, either in your team or another team. They maybe able help to talk to the senior.
-
Your last resort would be non tech management, or saying it another way: express that you’re not happy with your job. This won’t be much help unless others in your team doing so as well.
If all these fail, consider finding another offer. There’s no oversight, there’s no willing to inprove from the senior and there’s no chance to improve the situation from other seniors, you won’t learn much there.
-
If you’re following agile it is important for a team to agree on a definition of done for a story. If you don’t have one ask the scrum master to start that conversation or bring it up in a retro. One of the things that everyone can usually agree on is that the tests pass. Throw in a minimal coverage threshold as well. It’s not an indicator of good tests but it will tell you when there isn’t enough.
You mentioned that you’re doing this work for a client and that they will take over the code. Verify with management (in your company) if there are any quality measures specified in the contract. You don’t want your guy not performing up to the client’s expectations and you having to put in a lot of last minute nights and weekends to get there.
It’s reasonably common.
Teams fail together, even if it is the fault of an individual. Use your voice and don’t approve PRs if you don’t believe they are production ready.
I’d start rejecting his PRs lol, why is anyone but the original developer fixing his PRs?
I agree - if the reviewer doesn’t have the power to reject prs then they aren’t very useful reviews imo.
Right like, if we’re just doing PR reviews to do them ok but don’t I have anything better to do?
