코드 리뷰가 실제로 작동하게 만들기

주에 따라 다섯 명이거나 여섯 명이고, 저희는 만든 것을 직접 운영합니다. 별도의 운영 팀도 없고, 코드를 한 번도 만져 본 적 없는 사람들로 채워진 온콜 순번도 없습니다. 이 사실 하나가 리뷰 예절에 관한 어떤 글보다 제가 풀 리퀘스트를 읽는 방식을 결정했습니다. 목요일에 마이그레이션을 승인하는 엔지니어가 토요일 아침에 그 Postgres 로그를 읽는 엔지니어입니다.

코드 리뷰에 대한 글은 대개 더 큰 회사를 전제합니다. 코드를 쓰는 사람과 시스템을 책임지는 사람 사이의 관문, 대기열, 승인 정족수, 변경 하나에 몇 명의 눈이 필요한지에 대한 규정 같은 것들입니다. 이 정도 규모의 팀에는 그 관문 뒤에 서 있는 사람이 없습니다. 리뷰는 다른 방식으로 자기 몫의 시간을 벌어야 합니다.

팀이 다섯 명일 때 리뷰가 하는 일

저희는 한국에서 꽤 넓은 범위를 만들고 운영합니다. 커뮤니티 플랫폼, 그 안의 영어 우선 마켓플레이스, 7개 언어로 돌아가는 리테일 상담 도구, 그리고 대학생을 위한 지원 도구입니다. 사람 수보다 넓습니다. 그래서 아무도 한 서브시스템의 유일한 주인이 될 수 없습니다. 그 사람이 비행기를 타는 주에 그 서브시스템은 손댈 수 없는 것이 되기 때문입니다.

리뷰는 저희에게 세 가지를 해 주고, 값은 대략 이 순서입니다.

운영 맥락을 옮깁니다. 리뷰가 끝나면 최소 두 사람이 이 변경이 새벽 두 시에 무슨 짓을 하는지 압니다. 디프가 아니라 동작입니다. 무엇을 재시도하는지, 무엇을 기록하는지, 잘못됐을 때 로그에서 어떻게 보이는지. 제가 쓰지도 않은 PR을 읽는 이유가 전부 이것입니다.

테스트 스위트에 숨는 종류의 버그를 잡습니다. 대개는 로컬에 없는 데이터와, 작성자가 읽지 못하는 언어에 관한 버그입니다. 둘 다 아래에서 더 다룹니다.

결정을 찾을 수 있게 만듭니다. 6개월 뒤에 그 근거가 살아남아 있는 유일한 곳이 종종 코멘트 스레드입니다. 커밋 메시지는 스쿼시되고 문서는 낡지만, "여기서는 닫힌 쪽으로 실패시키기로 했다. 대안은 고객에게 틀린 가격을 보여 주는 것이기 때문이다"는 그것이 설명하는 줄에 붙어 남습니다.

리뷰가 하지 않는 일: 포매팅, import 순서, 이름 스타일, 그리고 타입 체커가 결정할 수 있는 모든 것. 그건 CI에서 돌고, 사람의 주의를 1도 쓰고 싶지 않습니다. 승인 개수도 원하지 않습니다. 리뷰어 풀이 세 명인 팀에서 승인 개수는 아무것도 측정하지 못합니다.

리뷰어가 곧 호출을 받는 사람입니다

저희 운영 환경은 자체 호스팅입니다. 뒤에서 조용히 장애 조치를 해 주는 관리형 데이터베이스도 없고, 저희보다 먼저 알아채 줄 플랫폼 팀도 없습니다. 변경이 잘못되면 여기 있는 누군가가 컨테이너를 재시작하고 로그 파일을 읽습니다.

그래서 리뷰가 운영 훈련이 됩니다. 디프를 읽으면서 실제로 스스로에게 던지는 질문은 이렇습니다.

• 이게 망가진 걸 내가 어떻게 알지? 쿼리가 실패해서 빈 목록을 반환하는 경로는, 보여 줄 게 없어서 빈 목록을 반환하는 경로와 화면에서 똑같아 보입니다. 그 차이가 로그에서 보이지 않으면, 저는 나중에 여기에 한 시간을 씁니다.
• 이게 틀렸다면 새벽 두 시의 해결책은 뭐지? 답은 셋이고, 값이 같지 않습니다. 설정값 하나 바꾸는 것은 괜찮습니다. 되돌리고 재배포하는 것은 받아들일 만합니다. "패치를 써서 배포한다"는 지금 더 들여다봐야 한다는 뜻입니다. 저는 새벽 두 시에 코드를 쓰고 싶지 않기 때문입니다.
• 이건 무엇을 얼마나 오래 붙잡고 있지? 바쁜 테이블에 배타 잠금을 거는 마이그레이션은 저희가 소유한 장비에서는 곧 장애입니다. 느린 것을 기다리는 동안 Redis 커넥션을 붙잡고 있는 요청도 마찬가지입니다.
• 이게 두 번째로 실행되면 어떻게 되지? 저희가 쓰는 모든 계층에서 재시도가 기본값입니다. 한 번은 안전하고 두 번은 파괴적인 핸들러가 리뷰에서 제가 찾는 가장 흔한 심각한 버그이고, 테스트에서는 거의 항상 보이지 않습니다. 테스트는 한 번만 호출하기 때문입니다.

이 중 어느 것도 코드 품질 얘기가 아닙니다. 이 변경이 살아남을 만한가에 대한 얘기입니다.

한 주를 낭비하는 코멘트

전부 제가 써 본 것들입니다. 그래서 쓸모없다는 걸 압니다.

첫째, 포매터가 이미 책임지는 코드에 스타일 의견을 다는 것. 도구가 불평하지 않았다면 저도 불평하면 안 됩니다.

둘째, 질문으로 위장한 재작성 요청. "여기 팩토리를 고려해 보셨나요?"는 리뷰어가 "이걸 바꿔라"라는 뜻으로 썼다면 질문이 아닙니다. 그러면 작성자는 그 코멘트가 막는 것인지 아닌지 하루를 들여 추측합니다. 둘 중 무엇인지 말하세요.

셋째, 리뷰 시점에 다시 여는 아키텍처 논의. 이 접근에 대해 제가 풀 리퀘스트에서 처음 들었다면, 실패는 며칠 전 아무도 묻지 않았을 때 이미 일어났습니다. 리뷰 스레드에서 그것을 다시 다투는 것은 작성자에게 한 주를 쓰게 만들고, 15분짜리 대화보다 나쁜 설계를 만들어 냅니다.

넷째, 가장 버리기 어려운 것: 코멘트 개수를 노력의 대리 지표로 삼는 것. 사소한 지적 열다섯 개에 실패 동작에 대한 질문 하나 없는 리뷰는, 꼼꼼해 보였고 아무것도 잡지 못한 리뷰입니다.

도움이 되는 코멘트와 아닌 코멘트의 차이는 대개 결과를 말하는가입니다. 실제 스레드에서 가져온 두 예시입니다. 약한 버전과, 쓸 값어치가 있었던 버전.

약함:  "이거 헬퍼로 빼는 게 어때요?"

나음:  "이 가격 포매팅 블록은 목록 시리얼라이저에도 있습니다. 반올림
        규칙이 바뀌면 둘 중 하나를 놓치게 되고, 검색 결과와 상세
        페이지의 값이 서로 어긋납니다. 병합 전에 한 벌로 만드는 게
        좋겠습니다."

약함:  "이거 잘못된 것 같은데요."

나음:  "carrierSubsidy가 null이면 여기서 그냥 통과해서, 실패하는
        대신 출고가를 최종 금액으로 반환합니다. 상담 화면에서는
        고객에게 너무 높은 가격이 표시되고, 매장에서 누군가 그걸로
        따지기 전까지 아무도 모릅니다. 지원금이 없으면 예외를 던져야
        하지 않을까요?"

두 번째 버전이 더 깁니다. 그리고 작성자는 저에게 아무것도 되묻지 않고 바로 움직일 수 있습니다. 무엇보다 제가 가장 중요하게 여기는 일을 합니다. 코드가 틀렸을 때 사용자가 무엇을 보는지 말해 줍니다.

장애를 막는 코멘트

세 가지 질문이 저희가 잡는 것 대부분을 찾아냅니다.

이 문자열은 사람에게 닿을 때 어느 언어인가

리뷰가 저희가 가진 다른 어떤 검사보다 확실히 나은 지점이 여기입니다. 버그가 디프에서는 훤히 보이는데, 자기 언어로만 앱을 열어 보면 실행 중에는 전혀 보이지 않기 때문입니다.

폰샵 도구는 상담 전체를 7개 언어로 그립니다. 학생 지원 도구는 영어로 쓰기 편한 서식을 받아 한국어로 제출 가능한 결과물을 만들어야 하고, 예시 답변은 두 언어로 나옵니다. 템플릿에 박힌 영어 라벨 하나는 린트를 통과하고, 타입을 통과하고, 테스트를 통과하고, 자기 브라우저에서 자기 로케일로 페이지를 여는 리뷰어도 통과합니다.

그래서 사용자에게 보이는 새 문구는 리터럴이 아니라 키를 받고, 저는 매번 리뷰에서 그것을 지적합니다. 조각을 이어 붙여 문장을 만드는 문자열 연결도 지적합니다. 한국어의 어순은 영어와 다르고, 그 조각들은 올바르게 재조립될 수 없기 때문입니다. 그리고 번역된 문구가 들어가는 고정 너비 요소에 대해 반드시 묻습니다. 가장 긴 번역은 스크린샷에 찍힌 그것이 아닙니다.

그 행이 없으면 이건 어떻게 되나

커뮤니티 데이터는 사용자가 만든 것이고, 시드 데이터가 결코 흉내 내지 못하는 방식으로 지저분합니다. 인원수가 끝내 채워지지 않은 그룹. 작성자가 계정을 지운 가이드. 더 이상 존재하지 않는 업로드 이미지를 가리키는 마켓플레이스 게시글. 로컬에서는 모든 것에 부모가 있습니다. 픽스처가 양쪽을 함께 만들었으니까요. 실서비스에는 고아가 존재하고, 그것이 없다고 가정한 코드 경로가 이제 500을 냅니다.

저는 모든 조인, 모든 선택적 외래 키, 그리고 두 필드를 읽고 둘이 일치한다고 가정하는 모든 자리에서 이것을 묻습니다.

절반쯤 실패한 뒤의 상태는 무엇인가

성공 경로도 아니고, 완전히 실패한 경로도 아닌, 그 가운데입니다. 채용 공고는 데이터베이스에 쓰였는데 알림이 실패한 경우. 파일은 업로드됐는데 그것을 참조하는 행이 끝내 커밋되지 않은 경우. 아무도 재현하지 못하는 문의 메시지를 만들어 내는 버그가 이 질문에서 나옵니다.

모바일 변경은 다른 리뷰를 받습니다

웹 배포는 몇 분이면 되돌립니다. 자체 호스팅 백엔드도 되돌릴 수 있습니다. 이미지를 다시 빌드하고 컨테이너를 재시작하면 이전 동작으로 돌아옵니다.

React Native 릴리스는 그렇지 않습니다. 잘못된 빌드는 스토어 심사를 지나 기기에 내려가고, 수정된 빌드가 심사를 통과하고 모든 사용자가 업데이트할 때까지 그대로 남습니다. 어떤 사용자는 영영 업데이트하지 않습니다. 그래서 모바일 풀 리퀘스트에서 리뷰어가 할 일은 다릅니다. 곧 영구적인 것이 될 결정을 전부 찾아내고, 그것이 서버가 바꿀 수 있는 값이어야 하지 않은지 묻는 것입니다.

실제로는 세 가지를 뜻합니다.

첫째, 위험한 것은 서버가 제어하는 플래그나 원격 설정값 뒤로 보냅니다. 그래야 수정에 스토어가 필요 없습니다. 둘째, 클라이언트는 아직 모르는 서버 응답을 견뎌야 하고, 서버는 몇 달 전에 배포된 클라이언트를 견뎌야 합니다. 새 필드는 추가되고, 오래된 필드는 같은 날 제거되지 않으며, 모르는 enum 값이 목록 화면을 죽여서는 안 됩니다. 셋째, API에 닿지 못할 때는 사용자가 재설치 없이 빠져나올 수 있는 상태로 실패해야 합니다.

세 번째는 잘못되기 쉬운데, 마침 리뷰가 잘 잡는 종류입니다. 오류를 처리하는 코드가 디프 안에 그대로 있고, 그것을 발동시키는 조건은 개발 중에 거의 일어나지 않기 때문입니다.

리뷰가 잡지 못하는 것

이것에 대해 정직한 것이 어떤 리뷰 체크리스트보다 중요합니다. 리뷰가 잡지 못하는 모든 실패는 다른 어딘가에 집이 있어야 하기 때문입니다.

마지막 행이 저희를 가장 아프게 했습니다. 운영 환경이 자체 호스팅일 때, 모든 변경의 일부는 저장소 밖에 삽니다. 환경 파일, compose 변경, 받아 오는 게 아니라 다시 빌드해야 하는 이미지 같은 것들입니다. 디프만 읽는 리뷰어는 사실 절반짜리인 변경을 완전한 변경으로 봅니다. 그래서 지금은 PR 설명이 배포 절차를 담고, 리뷰가 그것까지 포함합니다. "배포 절차 없음"이라고 적힌 풀 리퀘스트는 형식이 아니라 리뷰어가 확인하는 주장입니다.

뜻하는 바를 말하고, 승인하세요

저희는 평범한 어휘를 씁니다. 코멘트는 막는 것이거나, 제안이거나, 참고입니다. 실제로 중요한 부분은 리뷰어가 실패를 지목하고 작성자가 해결책을 고른다는 것입니다. 저는 밖에서 구현을 지정하고 싶지 않습니다. 제가 무엇을 두려워하는지를 작성자가 알기를 바랍니다.

코멘트를 달고 승인하는 것이 기본값입니다. 막는 것은 한 문장으로 말할 수 있는 정확성이나 안전 문제에만 씁니다. 다섯 명인 팀에서 취향 때문에 풀 리퀘스트를 막는다는 것은 기능이 한 사람의 기분을 기다린다는 뜻이고, 모두에게 리뷰를 거치지 말고 우회하라고 가르치는 일입니다.

리뷰 전의 리뷰가 이 모든 것보다 많은 일을 합니다. 작성자가 남에게 부탁하기 전에 자기 디프를 디프 화면에서 직접 읽는 것입니다. 제가 달았을 코멘트의 절반은 제 코드에서 그렇게 스스로 잡습니다. 그리고 PR 설명은 세 가지에 답합니다. 무엇이 바뀌는가, 이게 틀렸을 때 사용자가 무엇을 보는가, 어떻게 되돌리는가. 세 번째가 저희 프로세스 전체에서 값이 가장 큰 줄입니다. 답이 복잡하다면, 아무도 코드 한 줄 읽기 전에 중요한 것을 알아낸 겁니다.

여전히 놓치는 것

리뷰는 싼 것에 가장 강하고 비싼 것에 가장 약합니다. 고치는 데 2분 걸리는 번역 키 누락은 확실하게 잡고, 주말을 통째로 먹는, 데이터가 열 배가 되면 무너질 쿼리는 확실하게 놓칩니다.

코드가 아닌 변경은 덜 리뷰합니다. 가격 규칙 상수 하나, 새로운 콘텐츠 카테고리 하나, 설정 파일의 임계값 하나. 각각은 사용자에게 보이는 동작을 바꾸면서 디프에는 위험해 보이는 것이 하나도 없고, 아무도 깊이 생각하지 않은 승인과 함께 지나갑니다.

그리고 저는 여전히 밤 11시에, 오전 10시였다면 되물었을 것을 승인합니다. 이것의 정직한 해결책은 더 나은 체크리스트가 아닙니다. 피곤한 리뷰어는 리뷰어가 아니라는 것을 인정하고, 탭을 열어 둔 채 아침까지 두는 것입니다.