Hacker News new | ask | show | jobs
by koyote 1572 days ago
I only do it if I suspect that the code will not work. Sometimes I can think of edge cases that I suspect are not covered; so I check out the code, add the edge case as a unit test, and if it fails I mention it in the code review for the other dev to add and fix.

Other than that, no. I am reviewing the coding standard, architecture, and algorithms used. It's up to the dev to ensure that the code works and runs. The CI builds as well as the QA testers will test the actual functionality works as expected (obviously if I can't find the code that is supposed to fix/implement the PR item, I will question it).

1 comments

Wouldn't it be better to ask in the MR if they could just write a test to cover that case? It prevents wasting your time writing a test case that's going to be thrown out and prevents the other engineer from being blind sided by someone else committing a test case to their branch. I'm really not sold that the approach you're taking is of more value than just asking the MR author to add the test themselves.
That's a good point and you are right. I have simply asked for the engineer to write the test in the past, but sometimes my curiosity just gets the better of me and I want to try it out myself. Other times I am not quite sure what is wrong but it just "feels" wrong so I end up throwing a couple unit tests at it to see if my intuition was correct.

Note that this is quite rare and is maybe a once in a 100 code review occurrence. Usually it's quite obvious that something is wrong and you can just point it out or start a discussion about it.