Timeline for Should code reviewers reproduce the problem/solution as part of the code review?
Current License: CC BY-SA 4.0
32 events
| when toggle format | what | by | license | comment | |
|---|---|---|---|---|---|
| May 6 at 3:32 | comment | added | Eric Lippert | The most worrisome thing about your question is not that your management fails to understand code review, but rather than management has a completely wrongheaded understanding of the value of the QA department. Dev's job is to write correct, efficient, tested code that solves the customer's problem. QA's job is to advocate for the quality of the product on behalf of that customer; testing is one tool in QA's toolbox. Characterizing QA's primary purpose as verifying dev's unreviewed code is vastly missing the point. | |
| Apr 25 at 21:13 | comment | added | Filip Milovanović | Arbitrarily micromanaging how you work based on flawed assumptions is bad for business - and you should make damn sure they understand this. Especially if this boss is not a developer. You are the hired experts here. What you should do instead is try and understand the problem from the business perspective, discuss what the options are if they are not satisfied with the output, what kinds of risks are acceptable in terms of product quality, what kinds of things the team could do (including what kinds of tradeoffs must be made) in order to move the needle to a more desired state. 2/2 | |
| Apr 25 at 21:13 | comment | added | Filip Milovanović | And when QA returns faulty code, how does this boss imagine the fixes happen? Magic? God knows what they think "reproduce the problem" mean - they might be objecting to something that only exists in their head, cause they perhaps misunderstand what's actually going on. Don't assume that both of you mean the same thing under this term, and then go in with all kinds of arguments, it will just sound like a bunch of excuses. Instead, try to dig deeper and pull out of them what the actual concern is from their perspective, and if it's actually something to be concerned about. 1/2 | |
| Apr 25 at 18:21 | history | protected | gnat | ||
| Apr 25 at 17:54 | comment | added | Craig | I don't understand why this is an issue for management. Do they think PR reviews are taking too long? Are there conflicts over whether a PR actually fixes the problem? | |
| Apr 25 at 17:30 | comment | added | Greg Burghardt | I reverted the text I commented about just now. If this is what the OP meant, then the OP needs to edit their question to clarify. Note that at this point, such an edit would turn this into a different question. | |
| Apr 25 at 17:30 | history | edited | Greg Burghardt | CC BY-SA 4.0 | Revert last edit because it invalidates answers and fundamentally changes the context of the question. |
| Apr 25 at 17:28 | comment | added | Greg Burghardt | The most recent edit: "code reviewers, for instance when submitting code for a pull request," doesn't make any sense. Code reviewers do not submit pull requests; code authors do. The last edit invalidates answers and changes the context of the question. | |
| S Apr 25 at 16:54 | history | suggested | jonrsharpe | CC BY-SA 4.0 | Edit history is in https://softwareengineering.stackexchange.com/posts/457185/timeline |
| Apr 25 at 16:15 | answer | added | Robbie Dee | timeline score: 0 | |
| Apr 25 at 13:46 | review | Suggested edits | |||
| S Apr 25 at 16:54 | |||||
| Apr 25 at 13:00 | answer | added | candied_orange | timeline score: 0 | |
| Apr 25 at 8:22 | comment | added | pjc50 | TDD, especially purist TDD, isn't required, but a decent level of automatic test coverage pays for itself over and over. And yes this should happen in the CI/CD pipeline. Code reviews are usually just reading. | |
| Apr 25 at 8:09 | answer | added | Joop Eggen | timeline score: 0 | |
| Apr 25 at 5:15 | comment | added | amon | Code reviews may be a symptom, but it seems the underlying issue is that there's disagreement about what it means for a feature/change to be "done". You have no automated tests because there's no shared expectation of having tests (and presumably, because the software has evolved to be untestable). Because there are few tests and no comprehensive CI suite, some reviewers feel they have to do similar tasks manually. There is no right or wrong here, but there are tradeoffs – especially between delivering this one fix quickly vs ensuring long term maintainability. | |
| Apr 24 at 21:40 | review | Close votes | |||
| Apr 29 at 3:09 | |||||
| Apr 24 at 19:43 | answer | added | Thomas Owens♦ | timeline score: 7 | |
| Apr 24 at 18:59 | history | edited | Wes | CC BY-SA 4.0 | added 241 characters in body |
| Apr 24 at 18:38 | comment | added | Wes | @Ewan that is correct, code reviewer is not writing any code | |
| Apr 24 at 18:33 | comment | added | Ewan | "reproducing the ... solution" this means coding the feature to my reading? do you mean "verify the solution" | |
| Apr 24 at 18:30 | answer | added | Ewan | timeline score: 4 | |
| Apr 24 at 18:22 | vote | accept | Wes | ||
| Apr 24 at 18:20 | comment | added | Wes | @Ewan no we are merely reproducing the problem & solution (defects) or solution (features) to verify the above things I mentioned, but we (code reviewer) are not developing or re-developing anything. That is entirely the burden of the implementer, though we may have coding suggestions. | |
| Apr 24 at 18:20 | comment | added | Wes | @Ewan we = code reviewer, them = implementer | |
| Apr 24 at 18:18 | comment | added | Ewan | Are you literally re doing the work that has already been done? | |
| Apr 24 at 18:17 | comment | added | Ewan | who are we and them? | |
| Apr 24 at 18:10 | comment | added | Wes | @Ewan we have them run unit tests, however we do not always require them to write unit tests for their changes (generally they don't). Otherwise we do not require them to prove anything. Our unit testing coverage is extremely low so if there is something wrong with their solution there is virtually zero chance our unit testing will spot it. I have been trying to get our organization to address this lack of unit tests issue but with little luck. | |
| Apr 24 at 18:08 | comment | added | Ewan | talk me through it? doesnt the PR come with test proving it works? | |
| Apr 24 at 18:07 | history | edited | Wes | CC BY-SA 4.0 | added 62 characters in body |
| Apr 24 at 18:07 | comment | added | Wes | @Ewan that is correct | |
| Apr 24 at 18:06 | comment | added | Ewan | "code reviewers"? like people reviewing a PR? | |
| Apr 24 at 18:04 | history | asked | Wes | CC BY-SA 4.0 |