Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Authorization 헤더를 응답하도록 변경 #981
Authorization 헤더를 응답하도록 변경 #981
Changes from 4 commits
c4a885a
1766805
b61fdf2
a24bdcc
6e8bd43
8569298
d77eee0
320e655
b968ca2
f27c763
0a46b27
fae61a7
a278e16
873c9d1
9eb729a
41f5e1a
794a2e9
38b5f72
c917034
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
찾아보니
인증 정보가 없습니다. 다시 로그인해 주세요.
가 맞는 맞춤법이라네요~There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 리스트는 혹시 페어할 때 이야기 나누었던 것처럼 Adapter로 발전할 예정일까요?
Adapter 같은 일급 컬렉션으로 추출되면 적절한 CredentialManager를 반환하는지 테스트하기 더 좋을 것 같네요👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아마 프론트 코드 변경 이후엔 다시
List<CredentialManager>
->CredentialManager
로 변경될 것 같아요. 그래서 지금 예상은 Adapter 로 변경되지 않을 것 같네용There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오홍... 뭔가 우리 클라이언트의 환경이 무려 4개(웹, 익스텐션, vs 플러그인, intellij 플러그인)이기도 하고 ....!
호환성을 생각한다면 Mapping 객체를 하나 두는 게 더 낫다고 생각하긴 하는데 짱수는 어떤가요...!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아하,,, 그렇군요!!
그렇다면 지금처럼 여러개의
CredentialManager
가 공존해야 하는 상황이 지속될 수 있겠네용~~Mapping 객체 하나 더 두는 방향으로 수정해 보겠습니당~~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 작업을 #1010 이슈로 인가합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분의 코드가 ArgumentResolver의 코드와 완전 동일합니다.
즉, 파라미터로
@AuthenticationPrinciple Member member
만 추가해주고 아무 로직도 작성하지 않아도 동일한 동작을 하는데요.이 방향으로의 수정은 다들 어떻게 생각하시나요?
리팩토링 진행 시:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 좋다고 생각합니다~ 재사용할 수 있다면 하면 좋죠~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
member가 null로 들어온 경우에는 어떻게 동작할지 궁금해요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오~ 미처 생각을 못했군요~!!
Null 여부를 확인하여 예외 처리 추가해 두겠습니닷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드를 보고 생길 인지부조화도 훨씬 줄어든 것 같군요!! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
인수테스트를 작성한다는 관점으로 작성해 보았습니다.
통합 테스트를 작성하자는 논의 결과에 한번 작성해 보았는데, 편하게 많은 의견 남겨주시면 최대한 반영해볼게요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
클래스명에 Acceptance라는 단어가 들어가면 인수테스트라는 정보를 더 명확히 알 수 있을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 제우스 의견처럼 이름이 조금 더 명확하면 좋을 것 같아요!
AuthAcceptanceTest 좋네요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
정상적인 ID와 패스워드를 입력해야 로그인에 성공할 수 있다는 내용이 들어가면 좋겠어요~!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
헤더
,쿠키
는 인수테스트의 설명을 위해 사용하기에 적절한 단어가 아니라는 생각이 들어요.로그인 전에는 접근할 수 없는 자원에 로그인 이후 접근할 수 있게 된다는 것을 보여주는 테스트가 되면 좋을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 제우스 의견에 동의해요!
지금의 테스트는 컨트롤러 단위 테스트와 흡사해보이는 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음.. 뭔가 인수테스트의 개념이 명확하지 않은 것 같아요. 인수테스트의 개념에 대해 백엔드 내에서 정의를 해보면 좋을 것 같네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우선 제가 테스트 하고 싶었던 부분은 Cookie / Authorization 헤더 모두 정상적으로 credential 정보가 설정이 되는것이었어요.
즉, Controller 하나의 단위가 아닌 두 개의 CredentialManager 까지 함께 동작해야 하는 통합테스트에 해당한다고 생각했습니다.
다만, 헤더 / 쿠키 라는 이름이 인수테스트에 적합하지 않다는 의견에도 동의해요.
일단 인수테스트와 단순 통합 테스트를 분리해 두겠습니다!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
성공 테스트가 위에서 존재하는 Login의 성공 테스트와 함께 테스트가 됩니다. 그래서 작성하지 않았는데,,, 뭔가 이래도되나 싶네요.
"로그인이 잘 되었다" 와 "로그인 유지가 잘 된다" 를 따로 테스트 해야 할 필요가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그래도 api가 따로 존재하는 것이니 테스트 코드를 추가하는게 맞을 것 같아요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사용자의 로그인 상태가 유효한지 검증한다
라는 시나리오를 만들어 테스트하면 좋을 것 같아요.로그인 API와 로그인 확인 API의 목적은 다르니까요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로그아웃을 검증하기 위해 인증정보 쿠키의 이름이 노출되어야 합니다. (credential <- 쿠키 이름)
이 부분을 어떻게 해야 캡슐화 되어있는 쿠키의 이름을 재사용하지 않고 테스트를 잘 할 수 있을지 고민이네용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음,, 저희 DB 변수 숨기는 것처럼 yml 파일에 환경변수로 해당 변수를 등록해놓는건 어떤가요?
이 방법 외에는 떠오르지 않네요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
인수 테스트이므로 애초에 테스트하기 좋은 시나리오를 만드는 것도 방법이라고 생각해요.
로그인 상태에서는 정상적으로 접근할 수 있던 자원 또는 기능에 대해서 로그아웃 후에는 접근하지 못하게 하는 테스트가 되면 좋을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 인수테스트를 사용자에게 초점을 둔 테스트라고 이해했습니다.
그런 관점에서 쿠키 정보가 삭제되었는지를 사용자가 테스트할 필요는 없다고 생각해요.
제우스 말대로 로그인 전에는
내 템플릿
을 조회할 수 있었는데 로그아웃을 하고 나면 조회할 수 없는 시나리오 등으로 테스트 해봐도 좋을 것 같네요.근데 이게 인수테스트가 맞는지는 모르겠어요. 나중에 같이 인수테스트에 대해 얘기해봐요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아,, 설명이 조금 부족했네요!!
저도 이 의견에 공감합니닷! 다만, 문제가 되는 지점은 "쿠키 인증정보를 만료시키는 방법" 이에요.
지금의 동작은
SET-COOKIE
헤더를 사용해 기존의 인증 정보 쿠키를 만료시키는 방법인데요.이 동작은 브라우저에서 알아서 처리해주지만, 우리는 브라우저 없이 테스트를 진행해야 하기 때문에
SET-COOKIE
헤더를 확인하지 않고는 로그아웃을 테스트 하기가 어려운 상태라서 질문 드리게 되었어요~!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이것도 cm, 풀네임 둘 중 하나로 통일해주세오~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
헉 놓쳤다!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 클래스도 통합테스트를 위해 만들어 본 기반 환경이에요. MockMvcTest 에서 모킹 부분만 지운 상태입니다.
어때보이시는지,,,? 수정 보완 제안하실 부분도 편하게 말해주세요~~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일단 동작한다면 충분하다고 생각해요! 간결하고 좋네요 ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 개인적으로 통합테스트에서는 RestAssured를 사용하는 것을 선호합니다.
테스트의 가독성이 좋고 json 데이터도 더 쉽게 검증할 수 있기 때문입니다.
더군다나 RestAssured는 BDD 스타일로 작성할 수 있는데, 인수테스트가 사용자 시나리오를 테스트하는 것이라면 BDD의 장점을 더 잘 활용할 수 있다고 생각해요.
다른 분들 의견은 어떤가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 objectMapper는
@Autowired
를 사용하는 대신 직접 생성하는 이유가 뭘까요??There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오, 단순히 MockMvcTest 의 코드를 최대한 재활용했는데요!
생각해 보니 이 클래스는 실제로 사용되는 모든 클래스를 그대로 사용하기 위한 테스트이니
@Autowired
를 사용하는 것이 더 좋을 것 같네용