refactor: CallAdapter 적용 및 검증 테스트 작성 #558 #574

hxeyexn · 2024-12-20T16:27:12Z

⭐️ Issue Number

refactor: CallAdapter 적용 #558

🚩 Summary

CallAdapter 적용

CallAdapter 적용을 위해 Call, CallAdapter, CallAdapterFactory 객체를 구현했습니다.

CallAdapter 테스트 작성

MockWebServer를 활용해 CallAdapter가 정상 동작하는 지 검증했습니다.

ApiResult 처리 확장 함수 구현 및 Repository에 적용

Repository의 네트워크 응답 중복 로직을 최소화했습니다.

🛠️ Technical Concerns

CallAdapter

CallAdapter는 Retrofit2에서 지원하는 기능입니다. Retrofit은 HTTP 요청 및 응답을 처리하기 위해 기본적으로 Call 타입을 사용합니다.
CallAdapter는 이 기본 타입(Call)을 다른 타입으로 변환할 수 있도록 도와주는 역할을 합니다.

아래 4가지 클래스는 CallAdapter 구현을 위한 클래스입니다.

ApiResult(전 ResponseResult)
- 사용자 정의 응답 결과 sealed interfece(Success, ServerError, Exception)
ApiResultCall
- 웹 서버에 요청을 보내고 응답을 반환하는 객체
- Api 요청 결과를 ApiResult 객체로 반환할 수 있도록 구현한 Custom Call
ApiResultCallAdapter
- CallAdapter<R, T>는 Call<R>을 T로 반환해주는 interface
- CallAdapter 인스턴스는 Retrofit 인스턴스 안에 설치된 factory에 의해 생성
ApiResultCallAdapterFactory
- CallAdapter의 인스턴스를 생성하는 역할

ApiResult 처리 확장 함수 구현 및 Repository에 적용

Respository의 대부분의 메서드에 ApiResult 처리를 위한 보일러 플레이트 코드가 불편하게 느껴졌습니다.
따라서 ApiResult 처리 확장 함수를 구현해 보일러 플레이트 코드를 감소시켰습니다.

inline fun <T : Any, R : Any> ApiResult<T>.handle(convert: (T) -> R): ApiResult<R> {
    return when (this) {
        is Exception -> Exception(e)
        is ServerError -> ServerError(status, message)
        is Success -> Success(convert(data))
    }
}

테스트

JUnit5, AssertJ, MockWebServer, kotlinx coroutines test를 활용했습니다.
서버의 응답을 CallAdapter가 올바르게 처리하는지만 확인하면 되기 때문에 실제 서버에 요청을 보낼 필요는 없다고 판단했습니다.
MockWebServer를 이용해 응답을 가상으로 설정한 후에 ApiService 메서드의 반환값이 응답 결과에 따라 올바르게 반환 되는지 검증했습니다.
테스트는 HTTP status code 시나리오별로 작성했습니다.

⭐️⭐️ CallAdapter와 관련된 자세한 내용은 PR에 작성하기 양이 너무 많아서 Wiki [AN] CallAdapter 적용 및 테스트에 정리해뒀습니다.
아직 초안이라 부족한 부분이 많습니다. 틈틈히 퇴고하겠습니다..!

🙂 To Reviewer

ApiService의 모든 메서드들의 반환값이 ApiResult가 맞는 지 확인 부탁드리겠습니다.
CallAdapter를 처음 접한다면 원리를 이해하는 데 시간이 조금 걸릴 수도 있을 것 같습니다. 🥹
이해가 되지 않는 부분이 있으시다면 언제든 질문 주세요!
~~CI가 터지는 데 리뷰 주시는 동안 해결해보겠습니다..!~~ ➡️ 해결완료!

📋 To Do

CI에서 발생하는 문제 해결

- ResponseResult Exception의 message 파라미터에 기본 인자 설정

- 이전: handleApiResponse2 - 이후: handleApiResponse

- 이전: ApiResponseHandler - 이후: ApiResultHandler

- 이전: NetworkResultCall - 이후: ApiResultCall

- 이전: NetworkResultCallAdapter - 이후: ApiResultCallAdapter

- 이전: NetworkResultCallAdapterFactory - 이후: ApiResultCallAdapterFactory

s6m1n

해나 ! CallAdapter 적용하느라 수고 많으셨습니다 !
덕분에 중복 코드가 줄어들면서 훨씬 깔끔해졌네요. 게다가 꼼꼼한 테스트까지👍
올려주신 코드에 몇가지 개선점이나 질문들 남겨두었습니다..~

CI 때문에 넘 고생하셨어요 ㅋㅋ큐ㅠ 얼른 로그를 추가해보자구요!

s6m1n · 2025-01-13T09:14:13Z

android/Staccato_AN/app/src/main/java/com/on/staccato/data/ApiResult.kt

+
+class Exception<T : Any>(val e: Throwable, val message: String = EXCEPTION_NETWORK_ERROR_MESSAGE) : ApiResult<T>
+
+inline fun <T : Any, R : Any> ApiResult<T>.handle(convert: (T) -> R): ApiResult<R> {


해당 메서드에 inline 키워드를 사용하신 이유가 궁금합니다!

람다가 변수를 포획하면 람다가 생성되는 시점마다 새로운 무명 클래스 객체가 생기기 때문에 부가 비용이 듭니다.
이때 함수를 inline으로 선언하면, 함수를 호출하는 바이트 코드가 아닌 함수 본문을 번역한 바이트 코드로 컴파일됩니다!

그래서 실제로 decompiled.java 코드를 보면 앞서 말한 것과 동일하게 inline 유무에 따라 함수를 호출하는 부분이 다르게 컴파일 되는 것을 확인할 수 있습니다.

아래 사진은 MemoryDefaultRepository의 getMemory 메서드를 inline 키워드 유무에 따라 각각 디컴파일한 코드입니다.

inline 키워드가 붙은 handle 함수의 호출 부분은 handle 함수 본문이 그대로 바이트 코드로 번역되었습니다.

반면 inline 키워드가 붙지 않은 handle 함수의 호출 부분은 handle 함수를 호출하는 바이트 코드가 컴파일되었습니다.

추가로 직접 학습 테스트를 작성해 inline 유무에 따른 성능을 검사해봤습니다.
inline 키워드가 있는 handle 함수와 없는 handle 함수를 만들어 10번씩 무작위로 실행시키고, 실행 시간을 측정해보았습니다.

측정 결과 inline 키워드가 붙은 handle 함수가 그렇지 않은 함수보다 확연히 실행 시간이 짧았습니다. 10번 중에 1번 정도는 inline 키워드가 붙은 함수의 실행 시간이 더 긴 적도 있었지만, 90% 이상의 확률로 inline 키워드가 붙은 함수의 실행 시간이 더 짧았습니다.

또한 동일한 함수가 반복적으로 호출되면 실행 시간이 더 단축되는 것을 확인할 수 있었습니다.

첫번째 실행 결과

두번째 실행 결과

세번째 실행 결과

네번째 실행 결과

다섯번째 실행 결과

학습테스트 커밋: c414f4e

앞의 두 코멘트에서 설명한 이유 때문에 inline 키워드를 사용했습니다~

s6m1n · 2025-01-13T09:51:10Z

android/Staccato_AN/app/src/main/java/com/on/staccato/data/comment/CommentDefaultRepository.kt

 import com.on.staccato.domain.model.Comment
 import com.on.staccato.domain.model.NewComment
 import com.on.staccato.domain.repository.CommentRepository
+import kotlinx.serialization.json.JsonNull.content


이 친구는 린트와 CI도 잡아내지 못한 불필요한 import네요 ㅋㅋㅋ

저 친구는 어쩌다가... 린트한테도 안 잡히고 CI도 잡지 못한 걸까요..? ㅋㅋㅋㅋ 🤣
제거했습니다!

반영커밋: 9361ce8

s6m1n · 2025-01-13T10:12:03Z

android/Staccato_AN/app/src/main/java/com/on/staccato/data/ApiResultCallAdapterFactory.kt

+import java.lang.reflect.Type
+
+class ApiResultCallAdapterFactory : CallAdapter.Factory() {
+    override fun get(


로그를 찍어보니 get() 메서드는 returnType이 같은 경우 앱 실행 동안 한 번만 호출되는 것 같아요! 내부적으로 CallAdapter를 캐싱하나? 신기하네요...

맞아요! Retrofit 내부 구현을 살펴보면 callAdapter()와 nextCallAdapter() 메서드를 활용해 사용 가능한 팩토리 목록 중 returnType이 일치하는 CallAdapter를 찾아 반환하는 구조로 되어 있어요! 😄

이러한 구조라서 동일한 returnType에 대해서는 get() 메서드를 중복 호출하지 않는 것 같아요~

s6m1n · 2025-01-13T16:31:44Z

android/Staccato_AN/app/src/main/java/com/on/staccato/data/ApiResultCallAdapterFactory.kt

+    }
+
+    companion object {
+        fun create(): ApiResultCallAdapterFactory = ApiResultCallAdapterFactory()


companion object 안에 팩토리 메서드 create()를 구현하신 이유가 있을까요?
같은 맥락에서, StaccatoClient.kt의 addCallAdapterFactory에서 생성자가 아닌 create()를 호출하신 이유도 궁금합니다!

코드의 가독성을 위해서일까요...? 🤔 create() 메서드가 명시적으로 "생성한다"라는 의미를 나타내니까요!

ApiResultCallAdapterFactory의 생성 방식을 StaccatoClient가 구체적으로 알 필요가 없다고 생각했습니다.
즉, 추후 팩토리의 생성 로직이 변경 되더라도 외부에서는 create()만 호출하면 되기 때문에 수정 범위가 줄어듭니다.

이러한 이유로 Retrofit2의 ScalarsConvertFactory와 OptionalConverterFactory, KotlinSerializationConverterFactory 등도 create()를 활용해 Factory를 생성하고 있습니다!

ScalarsConverterFactory.java

OptionalConverterFactory.java

KotlinSerializationConverterFactory

s6m1n · 2025-01-13T17:03:42Z

android/Staccato_AN/app/src/main/java/com/on/staccato/data/ApiResult.kt

+
+class ServerError<T : Any>(val status: Status, val message: String) : ApiResult<T>
+
+class Exception<T : Any>(val e: Throwable, val message: String = EXCEPTION_NETWORK_ERROR_MESSAGE) : ApiResult<T>


Exception 클래스의 message 파라미터에 default 값으로 EXCEPTION_NETWORK_ERROR_MESSAGE를 넣어주셨군요!

이렇게 함으로써 상수를 한 곳에서 관리하고 코드 중복을 줄일 수 있다는 이점이 있지만,
과연 이 상수가 message의 default인자로 적절한지도 함께 고민해보면 좋을 것 같아요.
현재 구조에서 message 파라미터가 전달되지 않은 Exception 클래스는 항상 네트워크 불안정을 의미할까요?

Exception 클래스의 message 파라미터에 default 값으로 EXCEPTION_NETWORK_ERROR_MESSAGE를 넣어주셨군요!

CallAdapter 작업을 위해 Repository를 확인 했을 때 모든 Repository의 동반객체에 EXCEPTION_NETWORK_ERROR_MESSAGE이라는 상수가 있었어요. 그리고 Repository 내 모든 메서드의 Exception이 message로 EXCEPTION_NETWORK_ERROR_MESSAGE을 가지고 있었습니다!

저도 이 부분이 어색하다고 느껴졌습니다. 하지만 CallAdapter 적용이 우선이었기 때문에, 우선EXCEPTION_NETWORK_ERROR_MESSAGE 를 Exception 클래스 message 프로퍼티의 default 값으로 넣어두었어요.🙂

현재는 어색하다고 느낀 부분을 해결하기 위해 Exception을 세분화하고, ApiResultCall의 enqueue 메서드에서 onFailure 발생 시 전달된 Throwable의 종류에 따라 적절한 Exception 타입을 반환하도록 개선했습니다. 또한, ApiResult의 ApiResult<T>.onException() 확장 함수는 발생한 Exception에 해당하는 상태(ExceptionState)를 전달하도록 수정했습니다~ 😄

따라서 Exception 클래스의 message 파라미터 default 인자는 없어졌습니다!

작업커밋: 71fee22

Exception 세분화

sealed class Exception<T : Any> : ApiResult<T> { class NetworkError<T : Any> : Exception<T>() class UnknownError<T : Any> : Exception<T>() } inline fun <T : Any, R : Any> ApiResult<T>.handle(convert: (T) -> R): ApiResult<R> = when (this) { is Exception.NetworkError -> Exception.NetworkError() is Exception.UnknownError -> Exception.UnknownError() is ServerError -> ServerError(status, message) is Success -> Success(convert(data)) }

예외 상태 enum class

enum class ExceptionState(val message: String) { NetworkError(NETWORK_ERROR_MESSAGE), RequiredValuesMissing(REQUIRED_VALUES_ERROR_MESSAGE), ImageUploadError(IMAGE_UPLOAD_ERROR_MESSAGE), UnknownError(UNKNOWN_ERROR_MESSAGE), }

s6m1n · 2025-01-14T05:05:26Z

android/Staccato_AN/app/src/main/java/com/on/staccato/data/ApiResultHandler.kt

+
+import com.on.staccato.data.dto.Status
+
+suspend fun <T : Any> ApiResult<T>.onSuccess(executable: suspend (T) -> Unit): ApiResult<T> =


ApiResultHandler.kt의 onSuccess, onServerError, onException 확장함수들은 고차함수네요!
여기에도 inline 키워드를 추가해 무의미한 객체의 생성을 줄여볼 수 있지 않을까요?

LoginViewModel과 RecoveryViewModel에서 onSuccess 내의 action으로 사용자 토큰을 SharedPreferences에 저장하는 중단함수가 있어서 inline을 사용하기 애매했는 데 잠시 미뤄두고 수정한다는 걸 깜빡 했네요..! 의견 감사합니다!!

다시 코드를 살펴보니 사용자 토큰을 SharedPreferences에 저장하는 책임을 ViewModel이 가지고 있는 게 어색하다는 생각이 들었습니다. 사용자 토큰을 저장하는 로직은 SharedPreferences API를 사용하고 있기 때문에 Repository의 책임이라고 생각했어요 🙂

이 부분은 호두가 작업 했던 부분이라 호두와 의견을 나눈 후에 사용자 토큰을 저장하는 로직을 Repository로 이동시켰습니다!
위 리팩터링을 통해 ApiResultHandler.kt의 onSuccess, onServerError, onException 확장 함수들에 inline 키워드를 추가할 수 있게 되었습니다!

사용자 토큰 저장 리팩터링 커밋: 51954c3, 8a26c95
반영커밋: a554e09

s6m1n · 2025-01-14T05:07:51Z

android/Staccato_AN/app/src/main/java/com/on/staccato/StaccatoApplication.kt


 @HiltAndroidApp
 class StaccatoApplication : Application() {
    override fun onCreate() {
        super.onCreate()
-        // AppCompatDelegate.setDefaultNightMode(AppCompatDelegate.MODE_NIGHT_NO)
+        retrofit = StaccatoClient.initialize()


retrofit 초기화 위치가 StaccatoClient에서 StaccatoApplication의 동반 객체로 변경된 이유가 궁금합니다!

CI 오류 발생 원인과 관련 있습니다!

CI가 오류가 발생했던 시점을 돌이켜 보면, 테스트에서는 MockWebServer와 연결한 Retrofit을 참조하고 서버에서 들어오는 오류 응답을 변환하는 부분에서는 실제 서버와 연결된 레트로핏을 참조하고 있었습니다. 이로 인해 StacccatoClient을 초기화할 수 없는 문제가 발생했었습니다.

즉, 기존의 코드는 StaccatoClient 내부에서 Retrofit을 초기화하고 있었기 때문에 환경 별로 다른 Retrofit을 사용할 수 없는 구조였습니다. 이 문제를 해결하기 위해 StaccatoApplication의 동반 객체에서 retrofit을 지연 초기화하도록 변경했습니다.

s6m1n · 2025-01-14T05:13:20Z

android/Staccato_AN/app/src/main/java/com/on/staccato/data/comment/CommentRemoteDataSource.kt

-            handleApiResponse { commentApiService.postComment(commentRequest) }
+        override suspend fun createComment(commentRequest: CommentRequest): ApiResult<Unit> =
+            commentApiService.postComment(
+                commentRequest,


파라미터가 한 개일 땐 굳이 trailing comma를 붙이지 않아도 될 것 같아요!

제거했습니다~

반영커밋: d3bfb04

s6m1n · 2025-01-14T07:08:25Z

android/Staccato_AN/app/src/test/java/com/on/staccato/data/ApiResultCallAdapterTest.kt

+    }
+
+    @Test
+    fun `20MB를 초과하는 사진을 업로드 요청하면 오류가 발생한다`() {


20MB를 초과하는지 여부는 해당 테스트에서 검증하려는 부분이 아닌 것 같아요!
이 조건은 언제든 실제 서버에서 10MB나 30MB로 변경될 수 있습니다.

따라서 413 코드의 보편적 의미만 살려 '서버의 제한 용량을 초과하는 사진을 업로드 요청하면 오류가 발생한다'라고만 명시해도 충분할 것 같습니다!

20MB 초과 조건이 변경될 수 있다는 것을 고려하지 못했네요! 감사합니다~
반영했습니다!

반영커밋: 102d843

s6m1n · 2025-01-14T07:31:02Z

android/Staccato_AN/app/src/main/java/com/on/staccato/data/ApiResult.kt

+
+private const val EXCEPTION_NETWORK_ERROR_MESSAGE = "네트워크 연결이 불안정합니다.\n연결을 재설정한 후 다시 시도해 주세요."
+
+sealed interface ApiResult<T : Any>


sealed interface ResponseResult<T : Any> { class Success<T : Any>(val data: T) : ResponseResult<T> class ServerError<T : Any>(val status: Status, val message: String) : ResponseResult<T> class Exception<T : Any>(val e: Throwable, val message: String) : ResponseResult<T> }

기존에는 위처럼 ResponseResult의 중괄호 안에 포함되는 구조였는데, 모든 클래스가 최상위 레벨에 정의되면서 직접 접근이 가능해졌네요!

sealed interfacce 의 장점을 잘 살린 것 같습니다!
코틀린 최고야! 해나 최고야!

Junyoung-WON

해나!! 갓나!! 쵝오!!!👍👍
이번에 CallAdapter를 공부하면서 생각보다 복잡하다고 느꼈는데, 적용을 금방 끝내고 테스트까지 완벽하게 작성해주셨네요!
그리고 CI의 억까까지... 결국 모두 이겨내고 깔끔하게 해결도 해주셨군요! 정말 고생많았습니다 😭
해나가 프로젝트 초반부터 ResponseResult로 네트워크 요청에 대한 리팩터링을 해주었던 덕분에 지금과 같이 깔끔한 코드 구현이 가능해진 것 같습니다 💯💯

그렇게 미루고 미루던(?) CallAdapter PR을 드디어.. 끝냈습니닿ㅎ
역시 계속 자주 보고 공부해야 비로소 이해가 되는 것 같아요

빙티가 앞서 좋은 의견을 남겨주기도 했고, 크게 변경해야할 부분은 없을 것 같아 우선 Approve 드립니다!
사소한 의견과 질문 몇가지를 남겨놓았어요. 천천히 확인 부탁드려요~~

Junyoung-WON · 2025-01-15T10:23:52Z

android/Staccato_AN/app/src/main/java/com/on/staccato/data/ApiResultCall.kt

+private const val CREATED = 201
+private const val NOT_FOUND_ERROR_BODY = "errorBody를 찾을 수 없습니다."
+
+private fun <T : Any> handleApiResponse(execute: () -> Response<T>): ApiResult<T> {


CallAdapter가 직접 네트워크 호출의 대한 응답을 Call로 변환해주는 줄 알았는데, Call로 변환해주는 역할은 ApiResultCall에게 위임되었군요! 😲
결국 ApiResultCall의 역할은 Call를 Call<ApiResult>로 바꿔주는 것이네요~ 👍

한가지 궁금증이 생겼습니다!
Response의 결과에 따라 ApiResponse의 제네릭 타입을 지정하여 반환해주는 handleApiResponse 메서드는 ApiResultCall이 사용하기 때문에 ApiResultCall의 책임이라고 생각했어요. 그래서 해당 메서드가 ApiResultCall의 멤버 메서드로 들어가도 좋겠다는 생각이 들었습니다.
그렇다면 handleApiResponse 메서드를 ApiResultCall 클래스의 멤버로 두지 않은 이유가 따로 있으신건가요? 해나의 구현 의도가 궁금합니다~! 😄

응답에 따라 결과를 처리하는 부분은 Call의 역할이 아니라고 생각해서 handleApiResponse를 Call 외부에서 가지고 있도록 구현했습니다!

Junyoung-WON · 2025-01-15T10:35:26Z

android/Staccato_AN/app/src/main/java/com/on/staccato/data/ApiResultCallAdapterFactory.kt

+            return null
+        }
+
+        val callType = getParameterUpperBound(0, returnType as ParameterizedType)


CallAdapterFactory를 이해할 때, 변수명이 callType으로 되어있어서, Call의 타입을 의미하는 것인지 조금 헷갈렸던 것 같아요!

Call<T>의 파라미터 클래스 T의 타입을 획득하여 저장하는 변수이므로, 그 의미를 내포하는 변수명으로 바꾼다면 의미가 더 명확해질 것 같아요. 아래와 같은 변수명으로 바꿔보는 것은 어떨까요?

innerType : Wrapper인 Call의 내부 클래스 타입을 의미

parameterType : Call<T>의 파라미터 클래스 T의 타입

responseType : 네트워크 호출의 응답이 Call로 Wrapping되므로, 응답의 타입을 의미하는 responseType

DefaultCallAdapterFactory와, T를 Response로 감싸주는 CompletableCallAdapterFactory의 코드를 참고했습니다!

DefaultCallAdapterFactory의 get 메서드

CompletableCallAdapterFactory의 get 메서드

Call의 파라미터 클래스 T의 타입을 획득하여 저장하는 변수이므로, 그 의미를 내포하는 변수명으로 바꾼다면 의미가 더 명확해질 것 같아요. 아래와 같은 변수명으로 바꿔보는 것은 어떨까요?

너무 좋은데요?👍
ApiResult의 제네릭 인자를 얻는 부분에서 resultType이라는 변수명을 사용하고 있어서 responseType로 변경하는 게 좋을 것 같네요!

반영커밋: 36ec3e0

Junyoung-WON · 2025-01-15T10:43:19Z

android/Staccato_AN/app/src/main/java/com/on/staccato/data/ApiResultCallAdapterFactory.kt

+    }
+
+    companion object {
+        fun create(): ApiResultCallAdapterFactory = ApiResultCallAdapterFactory()


코드의 가독성을 위해서일까요...? 🤔 create() 메서드가 명시적으로 "생성한다"라는 의미를 나타내니까요!

Junyoung-WON · 2025-01-15T10:48:53Z

android/Staccato_AN/app/src/main/java/com/on/staccato/data/StaccatoClient.kt

+    fun Retrofit.getErrorResponse(errorBody: ResponseBody): ErrorResponse =
+        responseBodyConverter<ErrorResponse>(


정말..... 전혀 예상치 못한 곳에서 CI 에러가 나타난 원인이 여기에 있었다뇨... 😂
CI 로그를 뜯어보며 겨우 찾아내었네요! 정말 고생하셨습니다 👏👏👏

🫠🫠🫠🫠🫠

Junyoung-WON · 2025-01-15T10:51:15Z

android/Staccato_AN/app/src/main/java/com/on/staccato/data/ApiResult.kt

+
+private const val EXCEPTION_NETWORK_ERROR_MESSAGE = "네트워크 연결이 불안정합니다.\n연결을 재설정한 후 다시 시도해 주세요."
+
+sealed interface ApiResult<T : Any>


sealed interfacce 의 장점을 잘 살린 것 같습니다!
코틀린 최고야! 해나 최고야!

Junyoung-WON · 2025-01-15T10:57:33Z

android/Staccato_AN/app/src/main/java/com/on/staccato/data/module/RetrofitModule.kt

@@ -20,33 +20,33 @@ import javax.inject.Singleton
 object RetrofitModule {
    @Singleton
    @Provides
-    fun provideLoginApiService(): LoginApiService = StaccatoClient.create(LoginApiService::class.java)
+    fun provideLoginApiService(): LoginApiService = retrofit.create(LoginApiService::class.java)


이곳에서 Retrofit Service를 직접 create() 해주고 있었군요? 😆

Junyoung-WON · 2025-01-15T11:26:35Z

android/Staccato_AN/app/src/test/java/com/on/staccato/data/ApiResultCallAdapterTest.kt

+
+    @Test
+    fun `카테고리 생성 요청 중 서버의 응답이 없다면 예외가 발생한다`() {
+        mockWebServer.enqueue(MockResponse().setSocketPolicy(SocketPolicy.NO_RESPONSE))


이렇게 해서 MockWebServer의 응답 처리를 '응답 없음'으로 바꿀 수 있군요! 👍

Junyoung-WON · 2025-01-15T11:34:03Z

android/Staccato_AN/app/src/test/java/com/on/staccato/data/ApiResultCallAdapterTest.kt

스타카토 서비스 이용 중에 나타날 수 있는 다양한 상황을 예시로 들어서 요청 결과를 테스트하신거군요?
어떤 상황, 어떤 HTTP 응답 결과가 나왔는지에 따라서 CallAdapter가 무슨 ApiResult 로 변환해주는지 쉽게 이해할 수 있다는 장점이 보였어요!

하지만 한편으로는 ApiResultCallAdapter에 대한 테스트이니, 도메인이나 비즈니스 로직과는 별개의, 네트워크 통신을 중심으로 둔 테스트가 이루어져야 한다고도 생각이 들었습니다.
이에 대한 해나의 의견은 어떤지 궁금합니다! 그리고 이렇게 테스트를 작성하신 또 다른 이유가 있으신지도 궁금해요!

하지만 한편으로는 ApiResultCallAdapter에 대한 테스트이니, 도메인이나 비즈니스 로직과는 별개의, 네트워크 통신을 중심으로 둔 테스트가 이루어져야 한다고도 생각이 들었습니다.

테스트 명과 createMemoryResponse()와 같이 body를 생성하는 메서드 명을 말씀하시는 걸까요? 아니면 테스트 시나리오를 말씀하시는 걸까요? 이해가 잘 안돼서 여쭤봅니다..! 🥹

이에 대한 해나의 의견은 어떤지 궁금합니다!

테스트 명과 createMemoryResponse()와 같이 body를 생성하는 메서드 명을 말씀하신 게 맞다면, 이 부분은 네트워크 통신을 중심으로 둔 테스트라는 게 좀 더 드러나도록 테스트 명과 메서드 명을 변경하는 게 더 좋을 것 같다는 생각이 드네요~

그리고 이렇게 테스트를 작성하신 또 다른 이유가 있으신지도 궁금해요!

특정 status code들이 모든 상황에서 발생하는 것이 아니어서 그에 맞는 테스트 케이스를 고민하다보니 도메인명이 녹아진 테스트 명을 작성하게 된 것 같아요!

이런 형식으로 테스트용 FakeApiService를 만들어 사용하는 것도 괜찮을 것 같네요!

interface FakeApiService { // 200 @GET("/get/{id}") suspend fun get( @Path("id") id: Long, ): ApiResult<GetResponse> }

@Test fun `존재하는 데이터를 조회하면 데이터 조회에 성공한다`() { val success: MockResponse = createMockResponse( code = 200, body = createGetResponse(), ) mockWebServer.enqueue(success) runTest { val actual: ApiResult<GetResponse> = fakeApiService.get(id = 1) assertThat(actual).isInstanceOf(Success::class.java) } }

하지만 제 의견에 따라 테스트를 변경하기 위해서는, 네트워크 요청/응답에 대한 테스트용 Fixture를 추가로 구현해야할 것 같아요.
~~(세상에.. 귀찮을지도...?)~~
가짜 ApiService 와, 가짜 DTO 클래스가 필요할 것 같네요...
이 또한 관리의 영역이다보니, 테스트 유지보수 작업의 비용이 늘어난다는 단점이 나타나네요. 🥲

우선 아래와 같이 구현해 볼 수 있을 것 같아요. 임의로 작성한 코드이니, 적용한다면 조금 더 손을 보아야 할 것 같습니다!

interface FakeApiService { @GET(API_PATH_WITH_ID) suspend fun getRequest( @Path(ID) id: Long, ): Response<GetResponse> @POST(API_PATH) suspend fun postRequest( @Body postRequest: PostRequest, ): Response<Unit> @PUT(API_PATH_WITH_ID) suspend fun putRequest( @Path(ID) id: Long, @Body updateRequest: UpdateRequest, ): Response<Unit> @DELETE(API_PATH_WITH_ID) suspend fun deleteRequest( @Path(ID) id: Long, ): Response<Unit> companion object { const val API_PATH = "/" private const val ID = "id" private const val API_PATH_WITH_ID = "$API_PATH/{$ID}" } } data class PostRequest(val content: String) data class UpdateRequest( val id: Long, val content: String, ) data class GetResponse( val id: Long, val content: String, )

의견을 말씀드렸지만, 어느 방향이 좋을지 고민이 되네요.
빙티도 함께 고민해보면 좋을 것 같아요!
@s6m1n

하지만 현재 테스트에는 도메인과 비즈니스 로직이 녹아들어 있어요.
이 경우 어떤 상황에서 특정 응답이 나타나고, 또 ApiResult는 무엇이 반환되는지 알기 쉽다는 장점이 있습니다.
하지만 도메인과 비즈니스 로직이 변경되면, 이에 영향을 받는다는 단점이 있다고 생각했어요!
비즈니스나 도메인과 연관이 없고, 순수히 네트워크 요청 및 응답과 관련된 테스트 상황이 더 적절할 것 같다고 생각했습니다!

저도 정확히 같은 생각을 했습니다!
CallAdapter의 역할은 네트워크 응답에 따라 적절한 ApiResult 객체를 반환하는 것인데, 현재 테스트에는 이와 무관한 비즈니스 로직 정보가 드러나 있는 것 같아요.
CallAdapter의 역할 검증에 집중하려면 프로젝트에서 실제로 사용중인 ApiService 대신, 비즈니스 로직과는 무관한 테스트 전용 ApiService가 필요할 것 같네요!

좋습니다!

그럼 FakeApiService를 사용하는 방식 으로 변경하겠습니다~

반영했습니다!

반영커밋: 5eec9a6, 762a010, 5e96090

Junyoung-WON · 2025-01-15T12:07:47Z

android/Staccato_AN/app/src/test/java/com/on/staccato/data/FakeFileProvider.kt

+import okhttp3.RequestBody
+import okio.BufferedSink
+
+fun createFakeImageFile(): MultipartBody.Part = MultipartBody.Part.createFormData(name = "image", filename = "fileName", FakeRequestBody())


이렇게 해서 가짜 이미지 파일 객체도 만들 수 있군요?
Multipart 에 대한 테스트 객체를 만드는 방법도 알아가네요! 😄

Uri를 File로 변환해주는 기존 함수(convertMemoryUriToFile)를 사용하려고 하니 android 의존성 때문에 mocking 해야할 부분이 너무 많더라고요 🥲 그래서 가짜 이미지 파일 객체를 만들어 해결했습니다~

Junyoung-WON · 2025-01-15T12:21:47Z

...oid/Staccato_AN/app/src/main/java/com/on/staccato/data/staccato/StaccatoDefaultRepository.kt

+                        memoryId = memoryId,
+                        momentImageUrls = staccatoImageUrls,
+                    ),
+            ).handle { Unit }


빙티가 이야기한게 이 부분이었군요!
응답의 결과로 아무런 값이 반환되지 않는 경우에도 handle 메서드는 람다로 나타내야 하는군요...

handle { Unit } ➡️ 불편

handle { } ➡️ 얘도 불편

이 부분은 어쩔 수 없이 타협해야 할 것 같네요🥲

inline fun <T : Any, R : Any> ApiResult<T>.handle(convert: (T) -> R = { Unit as R }): ApiResult<R>

이런 식으로 default 인자를 추가하면 람다를 없애고 handle()로 사용할 수 있습니다!
그치만 응답에 반환값이 있는 경우에 handle()을 사용해도 컴파일러가 아무런 경고를 띄우지 않아 휴먼 에러의 우려가 있어요...
매우 불편하지만,, 어쩔 수 없을 것 같습니다...😅

맞습니다.. default 인자를 추가하면 람다를 없애고 handle()로 사용할 수 있지만 휴먼 에러의 위험이 너무 큽니다...!

그래서 조금 생각해봤는데, Unit을 처리하는 함수와 변환하는 함수를 따로 만드는 건 어떨까요? 때로는 중복 코드가 필요할 때도 있는 것 같아서요..!

@s6m1n

@Junyoung-WON

// 기존 handle 함수, 반환값이 있는 경우 inline fun <T : Any, R : Any> ApiResult<T>.transform(convert: (T) -> R): ApiResult<R> = when (this) { is Exception -> Exception(e) is ServerError -> ServerError(status, message) is Success -> Success(convert(data)) } // ApiResult<Unit> 처리 함수 fun ApiResult<Unit>.handle(): ApiResult<Unit> = when (this) { is Exception -> Exception(e) is ServerError -> ServerError(status, message) is Success -> Success(data) }

위와 같이 구현하면 Unit이 반환되는 경우에는 handle()로 사용할 수 있고, 반환값이 있는 경우에는 transform을 사용하도록 유도할 수 있습니다!

오...! 너무 좋은데요??
transform 이라는 메서드 명으로 훨씬 더 명확하게 나타내었네요! 😲 두 메서드가 구분되어있어서 혼용될 걱정도 없을 것 같구요!
완전 좋다고 생각합니다!!! 👍

Unit을 처리하는 함수와 변환하는 함수를 따로 만드는 건 어떨까요?

너무 좋습니다!

이건 추가 제안인데, 오버로딩을 이용하는 것은 어떠신가요?
transform이라는 이름으로 분리하는 것보다 handle이라는 공통 이름으로 처리하는 게 덜 헷갈릴 것 같다고 생각합니다.
컴파일러가 아래처럼 각 경우에 따라 적절한 시그니처의 메서드를 추천해주기 때문에 헷갈릴 염려도 없습니다!

반환값이 있는 경우

반환값이 없는 경우

오버로딩이 더 좋을 것 같네요! 오버로딩하는 방식으로 handle 함수 추가했습니다~

반영커밋: 084f788

- 서버의 이미지 제한 용량 변경과 관계 없도록 수정

- Call<T>의 파라미터 클래스 T의 타입을 획득하여 저장하는 변수라는 것을 명시적으로 나타내기 위해 변수명 변경

- delegate 패턴을 사용하고 있다는 것을 좀 더 명시적으로 나타내도록 변수명 수정

- 네트워크 중심으로 테스트가 이루어지도록 FakeApiService를 활용해 CallAdapter 동작 테스트 수정

- 생성 성공(201), 실패(400, 401, 500), 예외 - 네트워크 중심으로 테스트가 이루어지도록 FakeApiService를 활용해 CallAdapter 동작 테스트 수정

- 네트워크 중심으로 테스트가 이루어지도록 FakeApiService를 활용해 CallAdapter 동작 테스트 수정

linirini · 2025-01-19T11:03:33Z

안드의 꼼꼼함에 놀라고,,,,배워갑니다.....👍👍👍👍

- 이전: LoginViewModel에서 SharedPreferences에 사용자 토큰 저장 - 이후: LoginRepository에서 SharedPreferences에 사용자 토큰 저장

- 이전: executable - 이후: action

Junyoung-WON · 2025-01-20T07:48:33Z

android/Staccato_AN/app/src/main/java/com/on/staccato/presentation/util/ExceptionState.kt

+private const val REQUIRED_VALUES_ERROR_MESSAGE = "필수 값을 모두 입력해 주세요."
+private const val NETWORK_ERROR_MESSAGE = "네트워크 연결이 불안정합니다.\n연결을 재설정한 후 다시 시도해 주세요."
+private const val UNKNOWN_ERROR_MESSAGE = "예기치 못한 오류가 발생했습니다.\n잠시 후 다시 시도해 주세요."
+private const val IMAGE_UPLOAD_ERROR_MESSAGE = "이미지 업로드에 실패했습니다."
+
+enum class ExceptionState(val message: String) {
+    NetworkError(NETWORK_ERROR_MESSAGE),
+    RequiredValuesMissing(REQUIRED_VALUES_ERROR_MESSAGE),
+    ImageUploadError(IMAGE_UPLOAD_ERROR_MESSAGE),
+    UnknownError(UNKNOWN_ERROR_MESSAGE),
+}


ExceptionState를 만들고, handler 메서드가 ExceptionState를 반환해주도록 구현했네요!
data 레이어에서 사용자에게 보여줄 에러 메시지를 결정하는 로직을 잘 제거한 것 같습니다!! 완전 짱짱!! 😁

- 이전: RecoveryViewModel에서 SharedPreferences에 사용자 토큰 저장 - 이후: RecoveryRepository에서 SharedPreferences에 사용자 토큰 저장

hxeyexn added 30 commits December 19, 2024 07:04

refactor: ResponseResult interface와 응답 class 분리

4e11110

refactor: Exception message 기본 인자 설정

c992a87

- ResponseResult Exception의 message 파라미터에 기본 인자 설정

refactor: 204 응답 처리 로직 제거

d17777f

feat: NetworkResultCall 구현

3f60c5b

refactor: status code, exception 메시지 상수화

eb2ef9f

feat: NetworkResultCallAdapter 구현

ffe84f1

feat: CallAdapterFactory 구현

2ce3a7d

feat: Retrofit 초기화 시 CallAdapterFactory 추가

431975d

refactor: 타임라인 기능 CallAdapter 적용

b058b24

refactor: 카테고리(전 추억) 기능 CallAdapter 적용

d2e5e9d

refactor: 스타카토 기능 CallAdapter 적용

84b464f

refactor: 댓글 기능 CallAdapter 적용

535a3e9

refactor: 이미지 업로드 기능 CallAdapter 적용

b1e90fa

refactor: 마이페이지 기능 CallAdapter 적용

b334d4a

refactor: 로그인 기능 CallAdapter 적용

5c815fc

refactor: 멤버 기능 CallAdapter 적용

fc26942

refactor: 이전 handleApiResponse 제거

7501253

refactor: handleApiResponse2 이름 변경

3b534c2

- 이전: handleApiResponse2 - 이후: handleApiResponse

refactor: ResponseResult -> ApiResult로 이름 변경

9b9ad03

refactor: ApiResponseHandler 이름 변경

f846206

- 이전: ApiResponseHandler - 이후: ApiResultHandler

refactor: NetworkResultCall 이름 변경

b32ecc8

- 이전: NetworkResultCall - 이후: ApiResultCall

refactor: NetworkResultCallAdapter 이름 변경

be27563

- 이전: NetworkResultCallAdapter - 이후: ApiResultCallAdapter

refactor: NetworkResultCallAdapterFactory 이름 변경

7419d07

- 이전: NetworkResultCallAdapterFactory - 이후: ApiResultCallAdapterFactory

refactor: ApiResult 처리 로직 추가

8fcf08d

refactor: 카테고리 리포지터리 ApiResult 처리 확장 함수 적용

a9b13eb

refactor: 스타카토 리포지터리 ApiResult 처리 확장 함수 적용

63a9d7f

refactor: 타임라인 리포지터리 ApiResult 처리 확장 함수 적용

d01b05c

refactor: 마이페이지 리포지터리 ApiResult 처리 확장 함수 적용

d5c359c

refactor: 댓글 리포지터리 ApiResult 처리 확장 함수 적용

334d563

refactor: 멤버 리포지터리 ApiResult 처리 확장 함수 적용

f1605ee

s6m1n approved these changes Jan 14, 2025

View reviewed changes

Junyoung-WON approved these changes Jan 15, 2025

View reviewed changes

hxeyexn added 17 commits January 16, 2025 15:48

Merge branch 'develop' into feature/#558-apply-calladapter

b2b759f

style: 불필요한 import 제거

9361ce8

style: trailing comma 제거

d3bfb04

test: api 요청 실패(413) 시 CallAdapter 동작 테스트 명 변경

102d843

- 서버의 이미지 제한 용량 변경과 관계 없도록 수정

refactor: callType 변수를 responseType 으로 이름 변경

36ec3e0

- Call<T>의 파라미터 클래스 T의 타입을 획득하여 저장하는 변수라는 것을 명시적으로 나타내기 위해 변수명 변경

refactor: proxy 변수를 delegate 로 이름 변경

936f8b7

- delegate 패턴을 사용하고 있다는 것을 좀 더 명시적으로 나타내도록 변수명 수정

refactor: ApiResult<T> 처리 함수 표현식으로 변경

35ba213

test: 조회 성공(200) 테스트 FakeApiService 적용

5eec9a6

- 네트워크 중심으로 테스트가 이루어지도록 FakeApiService를 활용해 CallAdapter 동작 테스트 수정

test: 생성 성공/실패/예외 테스트 FakeApiService 적용

762a010

- 생성 성공(201), 실패(400, 401, 500), 예외 - 네트워크 중심으로 테스트가 이루어지도록 FakeApiService를 활용해 CallAdapter 동작 테스트 수정

test: 인증 정보 불일치(403) 테스트 FakeApiService 적용

63b2b33

- 네트워크 중심으로 테스트가 이루어지도록 FakeApiService를 활용해 CallAdapter 동작 테스트 수정

test: CallAdapterTest 내 사용하지 않는 ApiService 제거

5e96090

test: 테스트명 오타 수정

e869b50

test: dto 패키지 추가

b533341

test: Inline 학습 테스트 추가

c414f4e

style: 코드 formatting

12452ae

test: Inline 학습 테스트 assertAll 주석 처리

1d5e3e7

Merge branch 'develop' into feature/#558-apply-calladapter

7650d8c

hxeyexn modified the milestones: sprint-7, sprint-9 Jan 18, 2025

hxeyexn added 5 commits January 20, 2025 03:55

refactor: 예외 세분화 및 ApiResult<T>.onException이 예외 상태를 포획 하도록 수정

71fee22

Merge branch 'develop' into feature/#558-apply-calladapter

b39c759

refactor: LoginRepository에서 사용자 토큰을 저장하도록 변경

51954c3

- 이전: LoginViewModel에서 SharedPreferences에 사용자 토큰 저장 - 이후: LoginRepository에서 SharedPreferences에 사용자 토큰 저장

refactor: ApiResult<T> 확장함수 inline 키워드 추가

a554e09

refactor: ApiResult<T> 성공, 서버에러, 예외 별 처리 확장함수 파라미터명 변경

5db7be4

- 이전: executable - 이후: action

Junyoung-WON approved these changes Jan 20, 2025

View reviewed changes

hxeyexn added 2 commits January 20, 2025 16:55

refactor: MemberRepository에서 사용자 토큰을 저장하도록 변경

8a26c95

- 이전: RecoveryViewModel에서 SharedPreferences에 사용자 토큰 저장 - 이후: RecoveryRepository에서 SharedPreferences에 사용자 토큰 저장

refactor: ApiResult<Unit> 함수 추가 및 적용

084f788


		class Exception<T : Any>(val e: Throwable, val message: String = EXCEPTION_NETWORK_ERROR_MESSAGE) : ApiResult<T>

		inline fun <T : Any, R : Any> ApiResult<T>.handle(convert: (T) -> R): ApiResult<R> {


		class ServerError<T : Any>(val status: Status, val message: String) : ApiResult<T>

		class Exception<T : Any>(val e: Throwable, val message: String = EXCEPTION_NETWORK_ERROR_MESSAGE) : ApiResult<T>


		import com.on.staccato.data.dto.Status

		suspend fun <T : Any> ApiResult<T>.onSuccess(executable: suspend (T) -> Unit): ApiResult<T> =


		private const val EXCEPTION_NETWORK_ERROR_MESSAGE = "네트워크 연결이 불안정합니다.\n연결을 재설정한 후 다시 시도해 주세요."

		sealed interface ApiResult<T : Any>

		fun Retrofit.getErrorResponse(errorBody: ResponseBody): ErrorResponse =
		responseBodyConverter<ErrorResponse>(

refactor: CallAdapter 적용 및 검증 테스트 작성 #558 #574

Are you sure you want to change the base?

refactor: CallAdapter 적용 및 검증 테스트 작성 #558 #574

Conversation

hxeyexn commented Dec 20, 2024 • edited Loading

⭐️ Issue Number

🚩 Summary

CallAdapter 적용

CallAdapter 테스트 작성

ApiResult 처리 확장 함수 구현 및 Repository에 적용

🛠️ Technical Concerns

CallAdapter

ApiResult 처리 확장 함수 구현 및 Repository에 적용

테스트

🙂 To Reviewer

📋 To Do

s6m1n left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hxeyexn Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hxeyexn Jan 16, 2025 • 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

hxeyexn Jan 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hxeyexn Jan 20, 2025 • 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

hxeyexn Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hxeyexn Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Junyoung-WON left a comment

Choose a reason for hiding this comment

Junyoung-WON Jan 15, 2025 • 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

hxeyexn Jan 16, 2025 • 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

Junyoung-WON Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

hxeyexn Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

hxeyexn Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hxeyexn Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s6m1n Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

hxeyexn Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

hxeyexn Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

hxeyexn commented Dec 20, 2024 •

edited

Loading

s6m1n left a comment •

edited

Loading

hxeyexn Jan 16, 2025 •

edited

Loading

hxeyexn Jan 16, 2025 •

edited

Loading

hxeyexn Jan 19, 2025 •

edited

Loading

hxeyexn Jan 20, 2025 •

edited

Loading

hxeyexn Jan 16, 2025 •

edited

Loading

hxeyexn Jan 16, 2025 •

edited

Loading

Junyoung-WON Jan 15, 2025 •

edited

Loading

hxeyexn Jan 16, 2025 •

edited

Loading

Junyoung-WON Jan 15, 2025 •

edited

Loading

hxeyexn Jan 16, 2025 •

edited

Loading

hxeyexn Jan 16, 2025 •

edited

Loading

hxeyexn Jan 16, 2025 •

edited

Loading

s6m1n Jan 16, 2025 •

edited

Loading

hxeyexn Jan 16, 2025 •

edited

Loading

hxeyexn Jan 16, 2025 •

edited

Loading

Junyoung-WON Jan 15, 2025 •

edited

Loading

hxeyexn Jan 19, 2025 •

edited by s6m1n

Loading

s6m1n Jan 20, 2025 •

edited

Loading

hxeyexn Jan 20, 2025 •

edited

Loading