Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BE] 불필요한 컨텍스트 사용 제거 및 DI를 활용해 테스트 성능 개선 #734

Open
wants to merge 5 commits into
base: be/feature/729-test-refactor
Choose a base branch
from

Conversation

woosung1223
Copy link
Collaborator

@woosung1223 woosung1223 commented Nov 15, 2023

관련 이슈

구현 기능 및 변경 사항

  • 매번 실행하기에는 테스트 시간이 조금 부담스러운 것 같아, 실행 시간을 4.109s -> 2.625s로 개선했습니다.
  • Resolver 테스트라든지, Interceptor 테스트는 굳이 SpringBootTest로 진행할 필요가 없다고 생각해 POJO 단위 테스트로 변경했습니다.
  • 기존에 Access Token을 만료를 검증하기 위해 두었던 Thread.sleep(1000) 을 제거하고 DI를 활용해 테스트에서 구현체를 지정하도록 변경했습니다.

의견 있으시면 대면으로 말해주셔도 좋고, 아니면 코멘트 남겨주시면 감사하겠습니다!! 🙇‍♂️

스크린샷(선택)

  • 성능 개선 이전
image
  • 성능 개선 이후
image

@woosung1223 woosung1223 self-assigned this Nov 15, 2023
@woosung1223 woosung1223 added BE 백엔드 작업 refactor labels Nov 15, 2023
Copy link
Collaborator

@jaehee329 jaehee329 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제 PR에서 JWT 내용들이 삭제되며 무의미한 부분이 생길 것 같으니 확인해주실래요?


추가로 설명 남기자면 jwt 관련 내용 제거하면서 jsonwebtoken 관련 의존성을 제거하는 편이 좋을 것 같아요.
이번 PR에서 jsonwebtoken의 Clock을 적극적으로 사용하는 이유가 있을까요?
또 저번에 말씀드린 대로 제 PR에서 Interceptor에서 파싱한 내용을 캐싱해서 ArgumentResolver에서 쓰면서
이번 PR에서 만드신 stubbing 내역이 조금 불필요해질 것 같아요.
#729 PR에 리뷰 달았듯 제 PR과 기존 PR 머지 순서에 따라서 충돌이 많을 것 같아서 어떤 걸 빠르게 머지시키고 충돌을 해결하는게 어떨까요?

별개로 저도 Mockito를 쓰는 방향을 선호하고 Interceptor와 ArgumentResolver에 적용하는 것도 적절하다고 생각합니다.
전에 저희끼리 얘기하기로는 service 이상의 레이어에서 일관되게 SpringBootTest를 사용하기로 해 mockito 테스트가 없었던 걸로 기억해요. 앞으로 어떤 시점에 부분적으로 Mockito를 통한 테스트를 하는게 적절하다고 생각하시나요?

이외에 thread sleep 제거하는 부분은 당연히 저도 너무 좋습니다~
설명이 충분하지 않았던 것 같네요😅

@woosung1223
Copy link
Collaborator Author

추가로 설명 남기자면 jwt 관련 내용 제거하면서 jsonwebtoken 관련 의존성을 제거하는 편이 좋을 것 같아요.
이번 PR에서 jsonwebtoken의 Clock을 적극적으로 사용하는 이유가 있을까요?
또 저번에 말씀드린 대로 제 PR에서 Interceptor에서 파싱한 내용을 캐싱해서 ArgumentResolver에서 쓰면서
이번 PR에서 만드신 stubbing 내역이 조금 불필요해질 것 같아요.
#729 PR에 리뷰 달았듯 제 PR과 기존 PR 머지 순서에 따라서 충돌이 많을 것 같아서 어떤 걸 빠르게 머지시키고 충돌을 해결하는게 어떨까요?

별개로 저도 Mockito를 쓰는 방향을 선호하고 Interceptor와 ArgumentResolver에 적용하는 것도 적절하다고 생각합니다.
전에 저희끼리 얘기하기로는 service 이상의 레이어에서 일관되게 SpringBootTest를 사용하기로 해 mockito 테스트가 없었던 걸로 기억해요. 앞으로 어떤 시점에 부분적으로 Mockito를 통한 테스트를 하는게 적절하다고 생각하시나요?

이외에 thread sleep 제거하는 부분은 당연히 저도 너무 좋습니다~
설명이 충분하지 않았던 것 같네요😅

사실 JWT를 삭제하는 부분은 어느정도 염두에 두고 있었기 때문에 Thread.sleep을 개선할 때 고민을 많이 했었는데요(충돌이 날 것 같아)
말씀해주신 것처럼 Resolver나 Intercepter가 책임이 바뀌는 부분은 있는데, 사실 PR을 생성하는 시점에서 다른 opened PR을 매번 고려하기에는 비용이 큰 것 같아, 개인적으로는 merge 시점에 조율하는 것을 선호합니다. 그래서 일단 현재 코드에서 발행하는 문제를 개선한 것이었습니다! 오해의 소지가 있었을 수 있네요 😅

mockito에 관련해서는, 저는 mockito를 사용해야 한다기보단 Spring Bean을 사용할 필요가 없는 경우에는 빈 깡통 의존성을 Mock으로 대체하는 건 괜찮다고 생각합니다. 일부는 Mock 객체, 일부는 실제 객체가 필요한 경우는 현재 올린 PR과 같이 Spy로 해결할 수 있구요.

일관성을 위해 레이어마다 SpringBootTest를 사용할지 말지를 결정하는 것은 좋은 판단 흐름이라고 생각되는데, POJO 유닛 테스트가 가능한 상황에서는 컨텍스트를 띄우지 않아도 괜찮을 것 같습니다! 그리고 변경하기 이전 코드에서도 이미 MockBean을 사용하고 있어서요! 정책 상 바뀐 부분은 크게 없는 것 같아 걱정할 필요는 없을 것 같은데, 어떻게 생각하시는지 궁금합니다! 다른 분들의 의견도 남겨주시면 감사하겠습니다 😀

Copy link
Collaborator

@jaehee329 jaehee329 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다른 PR에 붙인 후에 충돌 해결하시죠~~

Copy link
Collaborator

@aak2075 aak2075 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확실히 컨텍스트가 줄어서 빨라졌군요 좋습니다 👍

private AuthService authService;
@Spy
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spy위에 개행 한 칸 해주면 좋을 것 같습니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BE 백엔드 작업 refactor
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants