Larvel 5.8에 contribution을 하다

1. 문제 발견

라라벨로 TDD를 하던 중, 분명 실패를 해야할 테스트 코드가 통과가 되는 이상한 현상이 있었습니다. 

Input validation을 테스트하는 과정에서 발견된 결함이었습니다.

 

예를 들어 아래와 같은 테스트가 있었습니다.

$response = $this->json('post', '/api/messages', [
    'body' => '',
    'receiver' => ''
]);

$response->assertJsonValidationErrors([
    'body' => 'required',
    'receiver' => 'required'
]);

 위 테스트는 의도적으로 body와 receiver라는 필드를 빈 문자열로 넘기고, 각 필드가 required라는 에러 메세지를 받는지를 테스트하는 코드입니다.

 

위 테스트는 잘 작동되는 것 처럼 보입니다. 하지만 문제는 아래와 같은 테스트도 통과를 했다는 것입니다.

$response->assertJsonValidationErrors([
    'body' => 'required',
    'receiver' => 'requiredddddd'
]);

이것은 확실히 이상한 현상인데, requireddddd라는 메시지는 절대 나올 수가 없기 때문입니다.

 

2. 디버깅

이후 여러가지 시도를 더 해봤습니다.

$response->assertJsonValidationErrors([
    'body' => 'requireddddd',
    'receiver' => 'required'
]);

문제의 메시지를 배열의 첫번째로 옮겨보니, 이번에는 정상적으로 테스트가 실패합니다.

왠지 배열의 첫번째만 검사를 하고 성공하면 그 뒤는 검사를 안하는 것다는 의심이 들었습니다.

 

그 의문은 assertJsonValidationErrors 메소드의 구현을 보고 확신으로 바뀌었습니다.

https://github.com/laravel/framework/blob/422754fb7b28fe6c75ee57f1dd476eaadc2a4c40/src/Illuminate/Foundation/Testing/TestResponse.php#L680

laravel/framework

첫번째 테스트가 통과가 되면 함수를 바로 종료시켜버립니다.

이 코드는 도대체 언제부터 이렇게 되어있었는지 궁금하여 git blame으로 확인을 해보니

두 달 전쯤 무려 Taylor Otwell(라라벨 창시자)가 작성한 코드였습니다..

(이 때 눈을 의심하며 혹시 이건 버그가 아니라 의도된건가 몇번을 다시 확인했습니다.)

 

 

알고보니 저 업데이트 이전에는 assertJsonValidationErrors가 아래와 같이 key값만 검사를 하였고 어떤 특정 validation error인지 message를 검사하는 것은 지원을 하지 않았습니다.

$response->assertJsonValidationErrors([
    'body',
    'receiver'
]);

 

버전 5.8.22부터 message를 선택적으로 넘겨주어 message까지 검사를 할 수 있게 업데이트가 되었는데,

작은 실수가 있었던 모양입니다.

(https://github.com/laravel/framework/releases/tag/v5.8.22)

 

3. 등록된 이슈가 있는지 확인

우선 이 상황에 대해 설명된 이슈가 있는지 찾아보았으나 나오지 않았습니다.

비교적 최신 버전인 5.8.22 버전 이후에 생긴 문제라 사용자가 적었을수도 있고,

또한 버그의 존재를 확인하기가 좀 어려웠을수도 있었던게 

문제가 없는데 있다고 하는 것은 크게 눈에 띄지만 문제가 있는데 없다고 하는 것은 인지하기 힘들지 않았을까라는 생각이 듭니다.

 

어쨋든, 이슈 등록을 할까 PR을 보낼까 하다가 문제의 원인까지 파악됐으니 PR까지 보내버리자고 마음을 먹었습니다.

4. 코드 수정 및 PR

간단한 문제였기에 수정을 하고, 혹시 유닛 테스트가 있나 싶어 코드를 뒤져보니 역시 있었습니다.

(테스트 코드를 테스트하는 코드..? 그럼 테스트 코드를 테스트하는 코드를 테스트하는 코드도 있나?)

 

테스트 코드들을 보니 테스트 데이터가 살짝 엉성한 감이 있었습니다.

테스트 코드엔 길이가 1인 배열에 대한 테스트만 있었는데 지금 버그는 두번째 값부터 확인을 안하는 문제였으니 기존 테스트 코드에서는 이 버그를 확인할 수 없었습니다.

 

제가 겪은 상황을 재현하는 테스트 코드를 추가하였습니다.

로컬에서 유닛 테스트를 돌려보고 모두 통과가 되는것을 확인한 다음 PR을 보냈습니다.

5. 마무리

https://github.com/laravel/framework/pull/29380

[5.8] Fix assertJsonValidationErrors with muliple messages by MoonHyuk · Pull Request #29380 · laravel/framework

Merged!!

 

이전부터 항상 Laravel같은 웹 프레임워크에 contribution을 하고 싶은 마음이 있었는데 마침내 하게되어 기분이 좋습니다. 아무래도 제가 항상 사용하던 프레임워크였고 직접 겪은 버그여서 쉽게 할 수 있지 않았나 싶습니다.

 

이 경험을 시작으로 앞으로 더 많이 기여를 하게 된다면 좋겠습니다. :)