#4 로그인기능 테스트/구현 #5

neropsys · 2021-04-25T12:21:53Z

No description provided.

detekt 에러 수정2

f-lab-hwan · 2021-04-26T12:49:14Z

코드 전반적으로 reformat 한번 적용해 주세요.

f-lab-hwan · 2021-04-26T12:51:29Z

app-core/src/main/kotlin/kr/flab/wiki/core/login/respository/UserLoginRepository.kt

+import kr.flab.wiki.core.login.persistence.User
+
+interface UserLoginRepository {
+    fun findByIdWithPassword(user: User):Boolean


isExist 같은 이름이 좀 더 적절할 것 같네요. 그리고 find 는 찾는다는 의미니까, 이렇게 바꾸면 어떨까요?

interface UserLoginRepository { fun isUserWithPasswordExist(user: User): Boolean = this.findByIdWithPassword(user) != null fun findByIdWithPassword(user: User): User? // ... }

인터페이스를 사용하는 쪽에선 유저데이터의 존재 여부를 if(isUserWithPasswordExist(user))로 표현할 지, if(findByIdWithPassword(user) != null)로 표현할지의 차이만 존재하기 때문에 리턴값을 boolean대신 nullable User를 리턴하는 메소드만 남기는것이 좋다고 생각합니다. 거의 같은 기능을 굳이 2개로 만들 필요는 없어보입니다

네. 말씀하신 방향도 좋아요.

f-lab-hwan · 2021-04-26T12:52:33Z

Service, Repository 등의 용어는 어떻게 도입하신건지, 그리고 각각의 의미는 무엇인가요?

만약 두개 이상의 Domain 을 다뤄야 하는 비즈니스 로직이 등장한다면 어떻게 이름을 짓는게 좋을까요?

app-core/src/main/kotlin/kr/flab/wiki/core/login/persistence/User.kt

f-lab-hwan · 2021-04-26T12:55:22Z

app-core/src/main/kotlin/kr/flab/wiki/core/login/business/UserLoginServiceImpl.kt

+
+class UserLoginServiceImpl(
+    private val userLoginRepository : UserLoginRepository,
+    private val sessionRepository: SessionRepository


Session 을 쓸 경우 예상되는 문제는 어떤 것이 있을까요?
(아직 다룰 주제는 아닙니다만 한번 예상해 보시기 바랍니다.)

Stateful해진다는 점, 그리고 다중로그인이 불가능하다는 점 이외에는 당장 생각나는 문제가 없습니다

f-lab-hwan · 2021-04-26T12:55:55Z

app-core/src/main/kotlin/kr/flab/wiki/core/login/business/UserLoginServiceImpl.kt

+        return userLoginRepository.save(user)
+    }
+
+    fun randomUUID():String{


UUID 는 16 바이트 (long long) 숫자인데 이걸 굳이 String 으로 표현해야 할 이유가 있을까요?

sessionRepository에서 세션의 key 값으로 문자열타입으로 저장합니다. 현재는 UUID(long long)타입을 키값으로 사용하지만 추후 UUID가 아닌 해시값과 같은 랜덤 문자열로 사용할 가능성을 염두해두었습니다.

f-lab-hwan · 2021-04-26T12:56:53Z

app-core/src/main/kotlin/kr/flab/wiki/core/login/business/UserLoginServiceImpl.kt

+) : UserLoginService{
+    override fun login(user: User): User? {
+        if (userLoginRepository.findByIdWithPassword(user)) {
+            sessionRepository.setAttribute(username = user.email, key = randomUUID())


세션에 저장한 사용자를 찾을 수단이 email 인 거죠?

이메일의 경우 각 유저 단위 고유값으로 활용할 수 있을것 같아서 세션정보를 email로 찾도록 구현했습니다.

f-lab-hwan · 2021-04-26T12:58:44Z

app-core/src/main/kotlin/kr/flab/wiki/core/login/respository/SessionRepository.kt

+
+interface SessionRepository {
+    fun setAttribute(key:String, username:String)
+    fun getKey(username: String):String?


getKeyByUsername 같은 이름에 대해서는 어떻게 생각하시는지 궁금합니다.

IDE상에서 메소드의 괄호를 열 때 미리보기로 패러미터의 이름을 보여줘서 굳이 username의 명칭을 붙이는것이 불필요하다는 생각이 들었습니다.

아하, 물론 말씀하시는게 맞기도 하지만 그 설정을 끈 사람은 그럼 그 혜택을 보기 어렵겠네요?

물론 kotlin 이기 때문에 getKey(username = "blahblah") 같은 형태로도 메소드 호출은 가능하니, 간결한 이름 유지하시는건 좋은 것 같습니다.

f-lab-hwan · 2021-04-26T12:59:11Z

app-core/src/test/kotlin/UserLoginServiceTest.kt

+import org.mockito.Mockito.*
+import java.util.*
+
+@Suppress("NonAsciiCharacters")


Suppress 에는 반드시 그 이유를 1줄 주석으로 추가로 달아주세요.

f-lab-hwan · 2021-04-26T12:59:34Z

app-core/src/test/kotlin/UserLoginServiceTest.kt

+    lateinit var mockSessionRepository: SessionRepository
+    lateinit var sut : UserLoginService
+    lateinit var user : User
+    @BeforeEach


각 test method 들은 한 줄씩 띄워 주시는게 보기 좀 더 좋습니다.

f-lab-hwan · 2021-04-26T13:00:56Z

app-core/src/test/kotlin/UserLoginServiceTest.kt

+    }
+
+    fun createRandomUser(): User {
+        val fakeValueService = FakeValuesService(Locale("en-GB"), RandomService())


이런 라이브러리 도입은 좋습니다. 다만 createRandomUser 는 이름이 좋은 의미를 가지는 만큼, test fixture 파일보다는 'test library' 같은 개념의 디렉토리나 모듈을 하나 만들고 그 쪽으로 옮기는게 좋겠네요. 테스트 코드에 테스트 시나리오 외의 코드를 최소화 하기 위한 practice 라 생각해 주시면 됩니다.

파일 이름이 굳이 java 스타일일 필요도 없습니다. testUtils.kt 같은 파일명도 좋겠네요.

f-lab-hwan · 2021-04-26T13:02:01Z

app-core/src/test/kotlin/UserLoginServiceTest.kt

+    }
+    @Test
+    fun `랜덤 유저가`(){
+        @Nested


이런 식의 테스트 구조화는 매우 좋습니다. 앞으로도 꾸준히 활용해 주세요.

f-lab-hwan · 2021-04-26T13:02:33Z

app-core/src/test/kotlin/UserLoginServiceTest.kt

+        }
+    }
+    @Test
+    fun `랜덤 유저가 미등록 상태에서 로그인 시도할 경우 널 리턴`(){


이 fixture 도 nested construct 안으로 넣어줄 수 있을 것 같네요?

제 테스트쪽 세팅에 문제가 있는지 nest화할 경우 테스트 결과창에서 랜덤 유저가부분의 코드만 실행이 되는지 로그에 랜덤 유저가 만 뜹니다. 어떻게해야 내부 nest 테스트코드까지 실행되는지 문의드릴려 했는데 잊어버렸네요ㅠ

app-core/src/test/kotlin/UserLoginServiceTest.kt

f-lab-hwan · 2021-04-26T13:04:17Z

app-core/src/test/kotlin/UserLoginServiceTest.kt

+        `when`(mockLoginRepository.save(user)).thenReturn(user)
+        //login 실행시 생성된 세션 uid를 리턴해줘야 함
+        `when`(mockSessionRepository.getKey(user.email)).thenReturn("nonNull")
+        assertTrue(sut.register(user) == user)


assert 가 여러 개 있는 test fixture 는 나쁜 test code 일 가능성이 높습니다. 어떻게 분리할 수 있을까요?

해당 테스트 한정해서 @before같은 기능을 쓰면 될 것 같지만 모든 테스트에서 실행이 되는 문제가 있습니다. 찾아보니 이런 방법이 있다고 하는데 다음에 도입해보도록 하겠습니다

음, 이런 경우는 assert 를 여러 줄로 쓰기보다는 assert 만 해 주는 전용 메소드를 하나 만들고 그 내부에서 모든 assert 를 수행하게 구성하는 방법도 있습니다.

그리고 test fixture 내에 여러 assert 가 있는 것이 좋지 않은 이유는, 중간에 assert 실패로 인해 그 이후의 테스트들이 모두 실패하기 때문이에요. 그 때문에 여러 항목을 assert 하는 로직은 테스트 실패 발생시 원인 복기에 시간이 많이 걸리게 됩니다.

ErrorCollector 같은 API 를 써서 그런 문제를 해결할 수 있긴 한데 테스트 코드가 다소 복잡해 지는 경향이 있으므로, 일단은 multiple assert 를 전용 static method 에서 수행하도록 코드를 변경해 주시는게 좋겠네요.

f-lab-hwan · 2021-04-26T13:05:20Z

buildScripts/detekt.gradle

@@ -0,0 +1,2 @@
+
+apply plugin: 'io.gitlab.arturbosch.detekt'


detekt 가 실제 빌드 process 에 맞물려 돌아갈 수 있도록 작업은 제가 하겠습니다.

FrancescoJo · 2021-04-26T15:20:11Z

app-core/src/main/kotlin/kr/flab/wiki/core/login/business/UserLoginServiceImpl.kt

+        return null
+    }
+    override fun register(user: User): User? {
+        return userLoginRepository.save(user)


이 대로라면 user 에 이상한 값 또는 무지막지하게 큰 값이 들어오더라도 무조건 허용할 것 같은데 이런 경우는 어떻게 방어할까요?

@kmmin78 님 같은 경우는 https://github.com/f-lab-edu/awesome-wiki/pull/10/files#diff-6a37c9f1fd4afe491a96b5a3a1a067576f27f66c3c07d6c80fdb28773f52f81dR29

이런 식으로 구현하셨던데, 좀 더 우아하게 해결할 방법은 없을까요? 한번 생각해 보시는게 좋겠네요.

FrancescoJo · 2021-05-03T11:17:02Z

@neropsys 요 PR 어떻게 진행할까요? 더 이상 수정 안 하실건가요?

neropsys

@neropsys 요 PR 어떻게 진행할까요? 더 이상 수정 안 하실건가요?

풀 리퀘에서 참조하는 브랜치에 추가 커밋을 올리면 풀리퀘에 반영이 안될 줄 알고 로컬의 브랜치에 별도로 작업을 하고 있었는데 반영이 가능한걸 몰랐습니다ㅠ 현재 브랜치에 피드백 반영하도록 하겠습니다

app-core/src/main/kotlin/kr/flab/wiki/core/login/persistence/User.kt

neropsys · 2021-04-28T16:38:08Z

app-core/src/main/kotlin/kr/flab/wiki/core/login/respository/SessionRepository.kt

+
+interface SessionRepository {
+    fun setAttribute(key:String, username:String)
+    fun getKey(username: String):String?


IDE상에서 메소드의 괄호를 열 때 미리보기로 패러미터의 이름을 보여줘서 굳이 username의 명칭을 붙이는것이 불필요하다는 생각이 들었습니다.

neropsys · 2021-04-28T17:29:23Z

app-core/src/test/kotlin/UserLoginServiceTest.kt

+        }
+    }
+    @Test
+    fun `랜덤 유저가 미등록 상태에서 로그인 시도할 경우 널 리턴`(){


제 테스트쪽 세팅에 문제가 있는지 nest화할 경우 테스트 결과창에서 랜덤 유저가부분의 코드만 실행이 되는지 로그에 랜덤 유저가 만 뜹니다. 어떻게해야 내부 nest 테스트코드까지 실행되는지 문의드릴려 했는데 잊어버렸네요ㅠ

neropsys · 2021-04-28T17:31:53Z

app-core/src/test/kotlin/UserLoginServiceTest.kt

+        `when`(mockLoginRepository.save(user)).thenReturn(user)
+        //login 실행시 생성된 세션 uid를 리턴해줘야 함
+        `when`(mockSessionRepository.getKey(user.email)).thenReturn("nonNull")
+        assertTrue(sut.register(user) == user)


해당 테스트 한정해서 @before같은 기능을 쓰면 될 것 같지만 모든 테스트에서 실행이 되는 문제가 있습니다. 찾아보니 이런 방법이 있다고 하는데 다음에 도입해보도록 하겠습니다

Author: neropsys <neropsys@gmail.com>

FrancescoJo · 2021-05-12T11:40:42Z

Possible duplicate of #16

neropsys added 8 commits April 25, 2021 14:30

인터페이스 추가

0a86c0b

인터페이스 정의 누락 부분 수정

56a70c7

gradle에 mockito, javafaker, hamcrest라이브러리 추가

d233257

gradle에 detekt 추가

18a0c7d

detekt 에러 수정

3cb82a8

detekt 에러 수정2

LoginRepository, LoginService 인터페이스 수정

6cde128

문자열타입 세션key리턴함수 추가

122f0c9

User객체 구현체, UserLoginService구현체, 테스트코드 추가

d9408a2

neropsys requested review from kmmin78 and FrancescoJo April 25, 2021 12:22

neropsys mentioned this pull request Apr 25, 2021

회원가입 기능 테스트코드 작성 #4

Closed

f-lab-hwan changed the title ~~Feature/hjchang/001 - 로그인기능 테스트/구현~~ #4 Feature/hjchang/001 - 로그인기능 테스트/구현 Apr 26, 2021

f-lab-hwan linked an issue Apr 26, 2021 that may be closed by this pull request

회원가입 기능 테스트코드 작성 #4

Closed

f-lab-hwan changed the title ~~#4 Feature/hjchang/001 - 로그인기능 테스트/구현~~ #4 로그인기능 테스트/구현 Apr 26, 2021

f-lab-hwan reviewed Apr 26, 2021

View reviewed changes

app-core/src/main/kotlin/kr/flab/wiki/core/login/persistence/User.kt Show resolved Hide resolved

f-lab-hwan reviewed Apr 26, 2021

View reviewed changes

app-core/src/test/kotlin/UserLoginServiceTest.kt Show resolved Hide resolved

f-lab-hwan reviewed Apr 26, 2021

View reviewed changes

f-lab-hwan removed the request for review from FrancescoJo April 26, 2021 13:05

f-lab-hwan assigned neropsys Apr 26, 2021

f-lab-hwan added the enhancement New feature or request label Apr 26, 2021

f-lab-hwan added this to CodeReview in awesome-wiki via automation Apr 26, 2021

f-lab-hwan removed this from CodeReview in awesome-wiki Apr 26, 2021

FrancescoJo reviewed Apr 26, 2021

View reviewed changes

Supress 용도 추가

c0af34e

neropsys commented May 3, 2021

View reviewed changes

neropsys added 3 commits May 3, 2021 20:51

reformat 적용

dbf9ae7

인터페이스 메소드 리턴값 변경

d9bda7c

Merge branch 'develop' into feature/hjchang/001

474246b

neropsys force-pushed the feature/hjchang/001 branch from d32ffdd to 3be890f Compare May 7, 2021 16:54

neropsys added 2 commits May 8, 2021 01:57

gradle 컨플릭 수정

d290fd7

Author: neropsys <neropsys@gmail.com>

mock객체 결과값 수정

25be998

Author: neropsys <neropsys@gmail.com>

neropsys force-pushed the feature/hjchang/001 branch from 3be890f to 25be998 Compare May 7, 2021 16:58

FrancescoJo closed this May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#4 로그인기능 테스트/구현 #5

#4 로그인기능 테스트/구현 #5

neropsys commented Apr 25, 2021

f-lab-hwan commented Apr 26, 2021

f-lab-hwan Apr 26, 2021 •

edited

Loading

neropsys Apr 28, 2021

FrancescoJo Apr 29, 2021

f-lab-hwan commented Apr 26, 2021

f-lab-hwan Apr 26, 2021

neropsys May 3, 2021

f-lab-hwan Apr 26, 2021

neropsys May 3, 2021

f-lab-hwan Apr 26, 2021

neropsys May 3, 2021

f-lab-hwan Apr 26, 2021

neropsys Apr 28, 2021

FrancescoJo May 4, 2021

f-lab-hwan Apr 26, 2021

f-lab-hwan Apr 26, 2021

f-lab-hwan Apr 26, 2021 •

edited

Loading

f-lab-hwan Apr 26, 2021

f-lab-hwan Apr 26, 2021

f-lab-hwan Apr 26, 2021

neropsys Apr 28, 2021

f-lab-hwan Apr 26, 2021

neropsys Apr 28, 2021

FrancescoJo May 4, 2021

f-lab-hwan Apr 26, 2021

FrancescoJo Apr 26, 2021

FrancescoJo commented May 3, 2021

neropsys left a comment

neropsys Apr 28, 2021

neropsys Apr 28, 2021

neropsys Apr 28, 2021

FrancescoJo commented May 12, 2021

#4 로그인기능 테스트/구현 #5

#4 로그인기능 테스트/구현 #5

Conversation

neropsys commented Apr 25, 2021

f-lab-hwan commented Apr 26, 2021

f-lab-hwan Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-lab-hwan commented Apr 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-lab-hwan Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrancescoJo commented May 3, 2021

neropsys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrancescoJo commented May 12, 2021

f-lab-hwan Apr 26, 2021 •

edited

Loading

f-lab-hwan Apr 26, 2021 •

edited

Loading