Code Review and part-time team members

Question

We have several part time testers who do both manual testing and test automation.

When I receive a pull request from them, I noticed that I am reviewing their code less strictly and allow a minor code style or organization violations to get through. My reasoning is that I know because they work in specific iterations, if I am going to request changes, this would add at least an extra day. Slowing down the turnaround time and increasing the merge conflict probability. Of course, but I do comment/reject if I notice major issues.

How problematic is this situation? Should I take into account that an author of the pull request works part-time or not?

Answer 1

First of all, whether a team member works part-time is irrelevant. Your team has a sprint backlog based on its velocity, which is variable for each sprint (dependent on team capacity for that sprint). If someone works only four hours a day, the workload for that person should be planned accordingly. Within the time allotted, everyone must do a proper job.

The question you should ask yourself is: are we building up technical debt by these loose reviews? Or, rephrasing this question: will these minor violations have to get fixed someday or cause quality issues (e.g. for test maintenance)?

If yes, you are also artificially raising your velocity and will run into issues later on.

• Everyone assumes the test is properly automated but the code quality is suboptimal.
• E.g. you commit a story of 10 points in 2 days, but when done properly would take 3 days.
• Not only does this set a similar expectation (or, in fact, limit) for all future stories, it also creates 'hidden' work (i.e. the technical debt that must be fixed but typically is not added to the backlog or status reports).

If you let these kind of things through but they are important in the long run, make them visible by creating backlog items for them!

Answer 2

Code style:

Code style checking should automated with a linter (e.g. ESLint and good style-guide like airbnb). No-one should be allowed to push code unless they ran the linter and fixed the coding linting errors. This should save you a lot of time.

Merge conflicts:

Merge conflicts should be solved by the coder. I would expect before the push the latest code would be merged into their code first. So it should not matter how long they take, merge conflicts should not be your problems, because how can you be sure how to solve them if you didn't code it. Normal code would have tests, but testcode doesnt... (hmmm, maybe we should test our code? ;-) Everyone should start their day with pulling the code from your main integration branch.

Code-reviews:

Code-reviews should focus on knowledge sharing and code-understandability.

If you find issues you have two options:

• Fix it yourself
• Send it back

I would send it back, even it costs a day extra in cycle-time. How would they learn and grow? If they are really a person you rarely use and getting them to grow is a waste, choose to fix it yourself. Don't let messy-code slip through. You will hate yourself in the longterm as you never make time to clean it later on.

Answer 3

No, part time or full time should not affect the quality of the work.

A well defined document with coding standards should solve this, it is never too late.

Any kind of work in any way is done, small or large sprints should have the same standards, not having a quality standard can result in poor work that can lead to more work when later you need to review it and maintain it.

Each project should have a wiki page detailing the process to be followed like coding standards, code reviews, adding tickets and so on.

I don't think can be a hard thing to follow few simple rules that can improve by a lot sometimes the quality of the work.

Maybe the following to iterations the things will be slower, but once they are use to the process everything should be back to normal, everyone wants a clean code review with no comments an no major things to change.