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.

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.

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.

I'd start rejecting his PRs lol, why is anyone but the original developer fixing his PRs?

Doesn't sound like a senior dev to me. Sounds like someone who thinks they are.

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.

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.

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!)

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.

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).

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.

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.

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

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!

80% of programming is maintaining existing code. His fragile code will be a problem in the future. This is not good .

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.

There are no complete, general statements that can be made about testing in all cases. How much converge and distribution of unit vs integration or white box vs black box, can not be generalized to all cases. The same is true of code practices.

However, policy can be black and white. And if the policy is to test or code with a specific style, then you need only point to that guidance and reject the patch. No confrontation or negativity needed. “This doesn’t currently meet policy x. Please fix and notify.” A respectable senior will appreciate that, and if they don’t they aren’t.

I would say it's how most softwares are done. And no, it's not a good thing, and you should do what you can to improve things.

My boss and his boss told me that it is impossible to keep documentation up to date and too expensive, which is why we don't have any. Let's say I'm not the most engaged worker there now.

I had this happen with a mid level guy (who thought he was a senior) with a cowboy attitude towards code. After many attempts to reason with him, I revoked his permission to touch the main branch and stopped reviewing his code until he started behaving. He left soon after.

In your case with a senior I'd escalate the situation to management.

This sounds like the story of how I got into CI/CD ….

I was working for a startup, and leadership was amazed at how “productive” this one developer was, changing thousands of files in marathon coding sessions. I realized he knew how to use the refactoring functionality in his IDE. More importantly, I was the QA peon tasked with being first one in every morning to characterize stability before everyone else started working. This guy and his antics made my life miserable.

However, I created build and test automation, and scheduled it nightly (this was before CI was a thing, before we had good tools), so now everyone got emailed the state of the product first thing every morning.

This diva was the only one making changes so all the instabilities were owned by him. Made my life much easier and helped our product get much better. More importantly, it did knock him down a peg, as the side effects of the scope of hos changes became more clear

I'm not super comfortable approving his work, but its functional and I don't want to hold up sprints...

I know it's not the point of your post, but this is a red flag to me. If you're using scrum (which it sounds like you are?), a sprint isn't defined as "when all the stories get to done", it's a set block of time (generally between 2 and 4 weeks). If the stories don't get to done in the time period, you don't hold up the sprint - they just didn't get to done. Most teams will just refactor the story into smaller pieces to carry over to following sprints.

I'd go for 1 on 1 discussion first. Understand their view. I assume you already tried.

Next level is a team discussion and understanding the team view. If you have sprints I assume you also have Retrospective. That's what it is for.

It's also not only for objective criteria and weighing cost and benefit, risk and effort reduction. It's also about you feeling comfortable with what you approve and ship. Even if not on a general level, at least in the project context with risks laid out and accepted.

Do you have a lead? Where I work senior indicates expertise through experience, with some decision making and guidance expertise. But lead would decide on direction or what is acceptable if it's not obvious or decidable on a collaborative team level.

Have a private conversation with him about this. Keep in mind that part of being a senior is knowing when it is ok to skip steps. So rather than calling him out, basically ask him to calm your discomfort by saying how what he is doing is justified.

Unit tests particularly are more helpful in dynamically typed or untyped languages than in typed ones. So if you are using typescript, you can get by with fewer unit tests. You still want good integration tests.

Added: I'm more awake now and see the part about your having to fix his broken tests. THAT is not good, unless you are jointly submitting the PR and a third person is reviewing. Assuming you are fixing the tests in his branch, make sure to comment (matter of factly, not accusingly) in the PR itself that you have fixed test #1, test #2, etc. That leaves some kind of trail that you did some of the work on that PR, which otherwise could be hard to find if you are squash merging so the commit history of his branch is not kept around. Then at evaluation time you can say something like "assisted so-and-so with test coverage on XYZ features". The idea is to avoid conflict but make sure you get credit for your work. Otherwise there is a PR with his name on it and nothing you can point to.

I worked with a cowboy-like dev a while back (very smart guy, knew the codebase like the back of his hand, but was given to hacky shortcuts) and I remember once or twice, I cringed at suggestions he made and said I wanted to be more consistent about datatypes etc. He nodded and understood, so I think I was a good influence.

How does your guy even submit a PR with broken tests? The workflows I'm used to won't let you submit a PR unless the tests pass.

There is a community !ask_experienced_devs@programming.dev that is another good place for this type of discussion.

Sounds like he might be more senior in age than in skill

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.

He is either over-worked and doesn't have time to abide by team standards, or lazy. If lazy, reject the PR for failing to meet standards.

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?