Coworker: Please review my PR.
-
Coworker: Please review my PR.
Me: [opens PR]
[CI build has failed with compile error]
Me: [attaching screenshot of build failing with compile error] I will review it when the build succeeds.

-
Coworker: Please review my PR.
Me: [opens PR]
[CI build has failed with compile error]
Me: [attaching screenshot of build failing with compile error] I will review it when the build succeeds.

I've had coworkers actually ship code that didn't even compile. The statically typed, pre-compiled code will build in the CI pipeline to create the deployable artifact. If that fails, it automatically blocks the PR, but JavaScript code is not checked in the CI pipeline. Unfortunately, a lot of it can't be, because it is dynamically generated by server-side rendering of pages. (My efforts to get rid of this went nowhere without support from leadership. They all agreed with me, but they did nothing.) Many times, I've had to comment something like "this doesn't look like it compiles". A few times, after pulling the branch and checking it myself, I've posted a screenshot of the compile error their code produces and "please test your code before requesting code review".
-
I've had coworkers actually ship code that didn't even compile. The statically typed, pre-compiled code will build in the CI pipeline to create the deployable artifact. If that fails, it automatically blocks the PR, but JavaScript code is not checked in the CI pipeline. Unfortunately, a lot of it can't be, because it is dynamically generated by server-side rendering of pages. (My efforts to get rid of this went nowhere without support from leadership. They all agreed with me, but they did nothing.) Many times, I've had to comment something like "this doesn't look like it compiles". A few times, after pulling the branch and checking it myself, I've posted a screenshot of the compile error their code produces and "please test your code before requesting code review".
In a situation like this, am I wrong to believe mandatory code review is necessary? Our pull request policies require that the PR's build succeed and a minimum of 2 approvals by team members who are neither the creator of the PR nor a commit author. Even still many code reviews are just rubber stamp approvals. I've seen PRs with failed builds that have two approvals. The time the coworker shipped code that had obviously not been tested because the JavaScript failed to compile, it had two approvals but clearly neither of them had reviewed it. Management has asked what we can do but never acted on any of the suggestions I've given. And yet, I like these people and feel like this has been one of the better places I've worked.
-
R relay@relay.infosec.exchange shared this topic