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

[Refactor/InhaBas#168]CookieUtilsTest 코드 리팩토링 #352

Closed
wants to merge 4 commits into from

Conversation

DongilMin
Copy link

@DongilMin DongilMin commented Sep 26, 2024

[feature/#21] 중복 코드를 제거하고, 상수를 사용함으로써 가독성을 개선

[feature/#38] JUnit방식의 전통적인 가독성이 떨어지는 코드를 개선

[feature/#52] JUnit방식의 전통적인 가독성이 떨어지는 코드를 개선

[feature/#72] JUnit방식의 전통적인 가독성이 떨어지는 코드를 개선

[feature/#85] thorws Exception으로 통합함으로써 코드 간결화

[feature/#88] 중복되는 난잡한 코드를 함수로 처리

[feature/#113] AsstertJ의 메서드로 수정

[feature/#106] 반복되는 cookie 설정 메서드 추가

[feature/#114]

  • OAuth2AuthorizationRequest.Builder를 사용하여 OAuth2AuthorizationRequest 객체를 생성하는 로직을 createOAuth2AuthorizationRequest 메서드로 분리
  • 중복 제거 및 가독성 개선

@skytin1004
Copy link
Member

@DongilMin 인텔리제이 터미널에서 ./gradlew :spotlessApply를 통해 코드 교정 부탁드립니다. 프로젝트 위치에서 한번 실행하면 됩니다.

@skytin1004
Copy link
Member

skytin1004 commented Sep 27, 2024

@DongilMin 로컬환경에서 gradle build 해보시고 모든 테스트 통과하는지 확인 부탁드립니다. 테스트 확인 결과 PR 메세지에 작성해주시면 감사하겠습니다. #165 NOTE 부분 참고 바랍니다.

public void deserializingTest()
throws NoSuchMethodException, InvocationTargetException, InstantiationException,
IllegalAccessException {
public void deserializingTest() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

기존에는 NoSuchMethodException, InvocationTargetException, InstantiationException,IllegalAccessException를 발생시키고 있는데 여기서는 Exception하나로 작동하고 있습니다. 이 부분은 다시 원래대로 바꾸는게 좋을것같습니다. 다른부분도 Exception 부분 다르게 되어있는지 점검해주시면 감사하겠습니다. 그리고 이렇게 바꿔야 하는 이유가 있으면 얘기해주시면 감사하겠습니다.

.additionalParameters(java.util.Map.of())
.attributes(java.util.Map.of("registration_id", "kakao"))
.build();
public void serializingTest() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

아래 리뷰랑 마찬가지로 체크 부탁드립니다.

@skytin1004
Copy link
Member

@DongilMin 가능하면 테스트 파일 더 묶어서 PR 제출해주실 수 있나요? auth관련된 다른 테스트 코드 있는지 확인해주시고 추가해서 PR 반영 부탁드립니다.

assertThat(deletedCookie).isNotNull()
.satisfies(cookie -> {
assertThat(cookie.getMaxAge()).isEqualTo(0);
assertThat(cookie.getValue()).isEqualTo("");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assertThat(cookie.getValue()).isEqualTo("");
assertThat(cookie.getValue()).isEmpty();

빈 문자열인지 확인하는 경우 isEmpty 사용할 수 있습니다.

@skytin1004 skytin1004 added the refactor Upgrade to better features label Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Upgrade to better features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants