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?
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.
Yeah that’s a major flag for me. If it’s just helping that’s one thing but they’re the senior dev so I’m not sure they should even need the help.
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.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
Why are you fixing his PR’s? Reject them for now following your own practices and link to the documentation about those practices that the PR violates.
You’re not holding up the sprint doing this, he is. As a team, you agreed these practices and everyone needs to follow them. If he refuses, raise it with his line manager.
Either his Line manager will put him in line, or he’ll agree that the standards you decided upon don’t need to be followed. Take your pick.
My opinion: don’t sweat it, either way. I know that’s easy to say from the outside, but it’s still true. Do what you are most comfortable with. It sounds like you have plenty of ammunition if you want to put your foot down & insist on quality practices. Reject PRs that don’t meet best practices, and point to the internal docs you have. If the dev reacts angrily, blame the company & say you are worried about getting in trouble.
Or if confrontation makes you more uncomfortable, just let it slide. If the shit hits the fan, the senior dev is the senior dev. Just say you were following their lead.
Above all, remember that the company you are working for is not your friend and not your ally. Look out for your own interests first & don’t stress about work as much as possible (I get that’s easy to say and tough to do, but it’s still the best idea!)
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!
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
There isn’t enough information here for me to say, but this MIGHT be similar to a self-organising dynamic I have seen before.
Maybe there is a dissertation somewhere in the dirth of programming team-dynamic books that has given it a name… But I just think of it like a medieval bulldozer.
Sometimes you have a headstrong dynamo who can and does crush through problems at a FANTASTIC rate. They have strong domain knowledge so everything is functionally correct. There may be some bugs (all code has bugs), but nothing that requires a re-design. But thier velocity is triple or quadruple everyone else’s.
But… This comes at a cost of things similar to what you said. A general disinterest for the “small things”, a reluctance to break their flow by going back for small bugs they missed. Skimpy test coverage. Since those things HAVE (asterisk) to get done eventually, they tend to pull less experienced devs into their gravity well, and they just end up in thier orbit applying thier full time efforts to patching the holes left behind.
That’s why I imagine them like a bulldozer in King Arthur’s court. They’re just a machine with the capacity to drive a mind boggling amount of blunt work, but require a small army of “finishers” to follow behind to add the finesse after the violence.
A few questions I would be mulling over and asking myself if I were in your situation:
Is this guy able to ship features orders of magnitude more quickly than his peers (regardless of style metrics?)
Does management seem to be aware but unconcerned?
If so, this is probably your situation. Your managers have a bulldozer and they figure it’s more effective to just let him do it and have you clean up after him.
It’s ACTUALLY a pretty sweet gig if you’re getting compensated well. You’re shielded from needing to make tough decisions, design decisions. You’re shielded from time crunches.
But… It’s maybe not super fulfilling. You might resent being in the shadow. You maybe want the opportunity to stake your own claim rather than just riding in this other person’s wake.
If that’s the case, I’d generally follow the advice given by others here… But make sure you truly understand the management dynamic before you start making moves that are too bold (such as, say, blocking PR merges that by convention were being merged in the past without anybody seeming to mind)… Because if right now management doesn’t see a problem, and you start refusing to merge PRs management will suddenly see a problem on their radar, and that problem will be you.
Honestly a frank discussion that you feel like your talents would be better applied to your own parallel work tasks rather than tagging behind the bulldozer in serial is probably the best way to go. You don’t need to shit on or diminish your coworker in order to plead your own case.
The truth is, as much as everyone in this conversation will hate to hear it, there is probably something you can learn from this person… If only how they were able to bend their environment so effectively into what they wanted
I’ve seen this play out a couple time. I agree about a lot of what you said and this is indeed true that you can have very senior and very knowledgeable devs basically “hack” or “bulldoze” their way into a backlog, I would personally argue that this is not a decent or desirable behavior.
There is no such thing as “small finition”. Making sure that a change or a feature works all the way through is not finition, it is core to the task, and you can’t expect someone else to digest and do the latter half of the work without being in your head.
I guess I am too lazy to type out all the examples with the downfall, but basically if you allow this, you will be shielding a senior from his own butched work, and lets be honest, most people who do this skip the “boring” work for their own selfish reasons. If they want to split the task and have you fix the tests, have them spell it out and justify it.
Management might not understand what is going on, all they might see is a superstar flying through the backlog, while everyone else struggle because they’re constantly fixing this guy’s shit. This rarely lead to good engineering, or team dynamic, or team management, and of course you end up with this one guy claiming credit for so much shit, while other team members stagnate. Unfortunately appearance is a thing in dev work, as much as I wish it wasn’t.
I’ve seen this play out many times, but only once was it good. BUT ONCE I did see it be good. It was interesting enough that I took the mental notes of why it worked. Huge asterisk because there are still pitfalls around the team having a single point of failure, but that’s an issue with many other modes with mixed skill.
Anyhow:
-The whole team was bought into it as a working mode
-There was a QA embedded directly into the team
-The bulldozer was forced, but willing, to routinely re-communicate plans and issues
-The bulldozer became good at proactively communicating “hotspots”
-The bulldozer was not allowed to do estimation, the surrounding team did that.
-The bulldozer agreed to be obligated to prioritize helping the team if they had questions (I think this is what helped him to be so proactive… He was incentivized to avoid this scenario of confusion entirely)
Anyways… I still don’t recommend it. But, assuming people are into it, I think there are ways to arrange the right individuals into teams in a way that minimizes the major pitfalls. I’m a pretty big fan of letting/helping teams self-organize into whatever their efficiency maximum is.
I don’t understand why you’d be fixing unit tests he broke during his pr. It seems like he might be bullying you? Maybe discuss with your manager.
This stuck out to me too. Why are you fixing things on their PR? If their changes broke the tests then they need to make the changes to fix them before merging
Unless it was directly caused by some code he wrote earlier that wasn’t caught at the time, he shouldn’t even consider that
even if it is an earlier, yet undeteced bug, whoever found it (in this case, the cowboy), should at least log it, if not open a separate PR to fix it.
Doesn’t sound like a senior dev to me. Sounds like someone who thinks they are.
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 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
I’m going to play devil’s advocate for a moment.
following best practices we laid out in our internal documentation
Are you absolutely sure those “best practices” are relevant or meaningful?
I once worked with a junior dev who only cared about “best practices” because it was a quickly whipped document they hastily put together that only specified stuff like coding styles and if spaces should appear before or after things. That junior dev proceeded to cite their own “best practices” doc with an almost religious fervor in everyone else’s pull requests. That stopped the very moment I made available a linter to the project, but mind you the junior dev refused to run it.
What’s the actual purpose of your “best practices” doc? Does it add any value whatsoever? Or is it just fuel for grandstanding and petty office politics?
his code works mind you,
Sounds like the senior dev is doing the job he was paid to do. Are you doing the same?
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.
Perhaps his job is to deliver value instead of wasting time with nonsense that serves no purpose. What do you think?
I’ve been dealing with similar things over the last few months.
Went through an org restructure, joined a different team and our tech lead was relatively new to the company but has 20yrs experience, sounded good!
Wrong.
Poor communication skills, endless pointless wrappers over everything and even simple things are wildly over-engineered with zero documentation, poor naming and zero tests to verify his homebrew solutions actually work or not.
We go live in 2 months for a nation-wide release. Send help.
I wonder, what kind of wrappers? I have seen some wrappers that seem useless at first, but they shine when we do a refactor because the wrappers concentrate logic in one place.
the pointless and poorly named kind.
Here’s one example of many:
class Group extends StatelessWidget { final CrossAxisAlignment crossAxisAlignment; final List children; const Group({ super.key, this.crossAxisAlignment = CrossAxisAlignment.stretch, required this.children, }); @override Widget build(BuildContext context) => Column( mainAxisSize: MainAxisSize.min, crossAxisAlignment: crossAxisAlignment, children: children, ); }For those unfamiliar with Flutter, Column is your go-to out of the box widget that’s already flexible and simple enough to implement and customize.
Groupexists purely to set a cross axis alignment value, and by name provides no indication as to what it does or how it lays out its children.All that just to save one line to override the default cross axis alignment…
