오픈소스 기여하기 회고 2 (feat. Regression Test)

안녕하세요, 이번 포스팅에서는 코드 관점에서 Django 버그 패치 과정을 다뤄보려고 합니다. 문제를 접근하는 방식, 해결 과정, 느낀점 등의 내용이 담겨 있습니다. 

 

버그 재현하기 (Reproduce bug)

우선 해당 이슈를 패치하기로 결정했다면 가장 먼저 해야할 일은 버그를 재현하는 것입니다. 대부분의 이슈들은 자세한 설명과 재현 방법이 나와있어 그대로 따라하면 됩니다. 만약 그렇지 않다면 이슈 리포터에게 조금 더 상세한 내용이나 샘플 코드를 부탁하면 됩니다.

 

제가 할당했던 티켓의 경우에는 명확한 재현 방법, version등이 나와 있어 재현에 어려움을 겪지는 않았습니다.

https://code.djangoproject.com/ticket/34052

#34052 (migrate --check still emits signals if database is up to date) – Django

 

이슈가 발생하는 원인 파악하기

이제부터는 이슈가 발생하는 원인을 코드를 분석해가면서 찾아야 합니다. 우선, 어느 코드에서 발생하는 문제인지를 가장 먼저 파악해야 합니다. 이 때 본인이 가장 분석하기에 편하고 좋은 방법을 이용하면 됩니다. 저의 경우에는 IDE의 기능중 하나인 Debugger를 붙여 이슈를 재현하면서 코드의 흐름을 보고 문제가 발생하는 위치를 찾을 수 있었습니다. (Migrate 커맨드의 수행중 발생하는 오류로, migrate.py 파일이지 않을까 라는 추측도 있었습니다)

 

처음에는 단순히 변수를 할당할 때의 조건문이 잘못 되었다 라고 판단해서 다음과 같이 수정을 진행했습니다.

# in /django/core/management/commands/migrate.py

plan = executor.migration_plan(targets)
...
# Before
exit_dry = plan and options["check_unapplied"]
# After
exit_dry = options["check_unapplied"]

위 코드에서의 plan은 Migrated가 되지 않은 마이그레이션 파일이 존재하면 [~~적용할 플랜1, 2, ..] 의 값을 가지고, 그게 아니라면 [ ] 빈 리스트의 값을 가집니다. options["check_unapplied"]의 경우 --check 옵션이 존재할 때만 True의 값을 가집니다.
여기서, exit_dry(sys.exit(1)을 유발하는 Boolean Flag 변수)의 값은 --check 옵션이 있을 경우 plan의 값에 따라 영향을 받지 않아도 된다고 생각했습니다. 

 

실제로 버그를 재현하는 과정에서도 해결이 된 것처럼 보였고, 기존에 존재하는 테스트도 통과하고 수정사항에 대한 테스트를 추가한 것도 통과하여 문제가 없는 것처럼 보였습니다.

 

그렇게 생각보다 쉽게..? PR을 오픈하고 기다리는 시간을 가집니다. 이번에는 하루가 지나지 않고 PR에 대한 코멘트를 받을 수 있었습니다. (굉장히 설렜습니다)

 

피드백 반영하기

하지만 PR에 남겨진 코멘트를 보고 무언가 완전히 잘못 이해하고 있었다는 사실을 깨닫게 됩니다. 바로 문서에 나와있는 옵션에 대한 잘못된 이해로부터 비롯되었습니다.

 

제가 이해한 내용 : 적용되지 않은 마이그레이션이 탐지되면 sys.exit(1) 수행.

-> 미적용된 마이그레이션 = "마이그레이션 파일이 있으나 미적용 + 마이그레이션 파일은 생성 안되었고 모델의 변경점이 있는 경우"

피드백 받은 내용 : 모든 마이그레이션이 적용되었다면 sys.exit(1) 미수행.

-> 미적용된 마이그레이션 = "마이그레이션 파일이 있으나 미적용"

 

바로 unapplied migrations의 범주에 대해서 잘못된 이해를 하고 있었던 것입니다. 그래서 아래와 같은 피드백과 남겨진 코멘트를 보고 한참을 생각하다가 아차 싶었습니다.

I don't thinks it's a proper patch, --check should return when all migrations are applied, not raise a SystemExit.

 

잘못된 범주를 바로 잡으니 생각보다 코드 수정이 꽤 있겠다는 것을 직감했습니다. 그리고 남겨진 코멘트를 토대로 수정하면서 논리적으로 더 수정할 부분은 없는지, 틀리진 않았는지를 재차 확인했던 것 같습니다. (메인테이너분의 e.g. 코드가 정말 많은 도움이 되었습니다.) 

 

 

두번째 남겨진 코멘트는 약간 신선했습니다. 항상 테스트를 강조하고 좋아하던 사람으로서 적잖은 충격과 신선함을 받을 수 있었던 코멘트 였습니다.

This is not a regression test as it works without the patch.

바로 제가 작성한 코드가 regression test, 즉 회귀 테스트가 아니라는 것이었습니다.

저는 제가 수정한 코드로 인해서 조건문의 진입에 대한 변경점이 일어날 것으로 예상되는 부분에 대한 테스트를 추가했습니다. 그러나 이 코멘트를 읽고 회귀 테스트가 무엇인가에 대해 한번 더 공부를 하고나니 잘못된 부분을 찾을 수 있었습니다.

 

바로 제가 수정한 코드베이스를 다시 rollback 한채로 Unit test를 실행했을때 추가한 테스트가 똑같이 통과하는 충격적인 결과를 확인할 수 있었습니다. 즉, 무의미한 테스트 코드였던 것입니다.

 

항상 코드 변경 & 테스트를 수정할 때는 코드 변경 -> 테스트 수정의 프로세스 즉, 코드를 기준으로 테스트를 작성했습니다.

 

하지만, 이번 기회로 인해 테스트가 기준이 되어서 코드가 변경되어야 한다는 TDD 관점에서도 생각해볼 수 있는 좋은 계기가 되었습니다. 또한, 그렇게 테스트를 강조했지만 실제로는 회귀 테스트를 잘 모르고 있었던 지금까지의 저도 많이 반성이 되는 부분이었습니다.

 

이후에 테스트를 수정하면서 회귀 테스트가 아닌 것들에 대한 것은 전부 삭제할 수 있었습니다. (4개의 테스트를 추가했는데 1개만 생존했습니다..)

이런 경험을 하고나니 기존에 작성했던 테스트도 무의미한 것들이 많지 않았나하고 돌이켜보게 되는 시간이었습니다. 앞으로의 리팩토링은 회귀 테스트의 관점에서 테스트를 작성해보도록 조금 더 신중을 기울여야 할 것 같네요.

 

짧은 PR, 많은 깨우침

그렇게 길지 않은 PR 이었음에도 불구하고 굉장히 많은 내용들을 배울 수 있었습니다. 개인적으로 크게 느낀 것은 철저한 코드 베이스와 Unit test가 지금까지의 Django를 잘 성장시켜오지 않았나 합니다.

 

이 외에도 느낀점들은 굉장히 많습니다. 다음 포스팅에서 후기를 마지막으로 이번 시리즈를 마쳐볼까 합니다.

 

읽어주셔서 감사합니다 :)