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?
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 [email protected] that is another good place for this type of discussion.