10
momad
49d

Would telling someone in a code review that their code wouldn't even work and that a simple test would have caught it of they wrote one be considered rude? Should I just tell them exactly what the problem is and move on or make them figure it out?

I want people to be testing the code they write but I don't want to just become annoying to them by telling them constantly to add tests when there is no immediate payoff that these juniors obviously crave. Similarly I don't want to antagonise them when they are so early in their career.

Comments
  • 7
    You can just tell them that it's not working and follow it with a "moving forward, run the tests first". Also, aren't tests required in your project? You can reject their code if it's incomplete.
  • 1
    Ask them to write a test for it before you'll approve it.

    Works every time.
  • 4
    I usually go with:

    "Hi, we seem to be missing tests here - as per our code quality guidelines, can you add thorough tests to all non-trivial methods and ensure they pass?"

    You can go the route of using automated rules to test if coverage on the PR is above a threshold too, but I usually avoid that as the over-reliance on a number creates way more problems than it solves in my experience.
  • 2
    Just ask them to test their stuff before pushing or creating a pull request for the main branch.
    If it doesn't work on staging, chances are, it also doesn't work on their dev machines.

    While tests are in general nice to have, there might be higher priority things to do first.
    But with or without tests, some manual testing should always be required before pushing new code to staging.
    Humans are bad at testing. But they are even worse on writing good code - and tests like documentation are code too.
Add Comment