[Feat] Roborazzi + Robolectric 기반 스크린샷 테스트 도입#476
Open
Conversation
PeraSite
changed the title
Contributor
Summary of ChangesHello @PeraSite, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 Pull Request는 앱의 UI 안정성과 테스트 용이성을 크게 향상시키기 위해 포괄적인 스크린샷 회귀 테스트 프레임워크를 통합합니다. 이를 통해 개발 과정에서 시각적 회귀를 조기에 감지하고, 비결정적 UI 요소에 대한 테스트 신뢰도를 높이며, 광범위한 단위 테스트를 추가하여 전반적인 코드 견고성을 강화했습니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
이번 PR은 Roborazzi와 Robolectric을 이용한 스크린샷 테스트 체계를 도입하여 앱의 UI 안정성을 크게 향상시키는 중요한 변경입니다. 결정성 확보를 위한 테스트 seam 도입, 시간 관련 로직의 테스트 용이성 개선, 그리고 방대한 양의 단위 테스트 및 UI 테스트 추가 등 인상적인 작업입니다. 특히 커버리지 가드를 통해 테스트 누락을 방지하는 구조는 매우 훌륭합니다. 몇 가지 발견된 버그와 개선점에 대한 의견을 남겼으니 확인 부탁드립니다.
Comment on lines
+131
to
+144
| `when`("writeMenuReview에서 likeMenuIdList가 빈 리스트면") { | ||
| then("현재 구현 그대로 NoSuchElementException이 발생한다") { | ||
| runTest { | ||
| shouldThrow<NoSuchElementException> { | ||
| repository.writeMenuReview( | ||
| rating = 1, | ||
| content = "x", | ||
| imageUrls = emptyList(), | ||
| likeMenuIdList = emptyList(), | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
이 테스트 케이스는 likeMenuIdList가 비어 있을 때 NoSuchElementException이 발생하는 버그를 잘 보여주고 있습니다. writeMenuReview 구현에서 빈 리스트에 .first()를 호출하는 것으로 보입니다. likeMenuIdList가 비어 있거나 null일 경우, 요청 시 menuLike 필드를 null로 설정하는 등 예외 상황을 안전하게 처리하도록 수정하는 것이 좋겠습니다.
Comment on lines
+91
to
+111
| `when`("수정이 실패하면") { | ||
| val useCase2 = mockk<ModifyReviewUseCase>() | ||
| val viewModel = ModifyViewModel(useCase2) | ||
| viewModel.init(4, "old", likes) | ||
| viewModel.onContentChanged("new") | ||
| coEvery { useCase2(11L, 4, "new", any()) } returns false | ||
|
|
||
| then("현재 동작(characterization): 실패 토스트 후에도 뒤로가기+성공 토스트를 보낸다") { | ||
| runTest { | ||
| viewModel.uiEvent.test { | ||
| viewModel.submit(11L) | ||
| advanceUntilIdle() | ||
|
|
||
| awaitToastEvent().assertToast(R.string.toast_review_modify_failed, ToastType.ERROR) | ||
| awaitItem() shouldBe UiEvent.NavigateBack | ||
| awaitToastEvent().assertToast(R.string.toast_review_modify_success, ToastType.SUCCESS) | ||
| cancelAndIgnoreRemainingEvents() | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
이 테스트는 리뷰 수정 실패 시 실패 토스트와 성공 토스트가 모두 발생하는 버그를 발견했습니다. 실패 시에는 실패 처리 로직만 수행하고 성공 흐름으로 넘어가지 않도록 수정해야 합니다.
Comment on lines
+10
to
+14
| @Volatile | ||
| private var forceEnabled: Boolean = false | ||
|
|
||
| val isEnabled: Boolean | ||
| get() = forceEnabled || System.getProperty(PROPERTY_KEY) == "true" |
Contributor
There was a problem hiding this comment.
@Volatile 필드인 forceEnabled를 사용하여 테스트 모드를 제어하는 방식이 다소 복잡하며, 잠재적인 동시성 문제를 야기할 수 있습니다. ScreenshotDeterminismRule에서 이미 시스템 프로퍼티를 설정하고 있으므로, forceEnabled를 제거하고 시스템 프로퍼티에만 의존하도록 단순화하는 것이 좋겠습니다. 이렇게 하면 정적 가변 상태를 제거하여 코드를 더 견고하고 예측 가능하게 만들 수 있습니다.
enableForTest와 disableForTest에서도 forceEnabled 관련 코드를 제거해야 합니다.
Suggested change
| @Volatile | |
| private var forceEnabled: Boolean = false | |
| val isEnabled: Boolean | |
| get() = forceEnabled || System.getProperty(PROPERTY_KEY) == "true" | |
| val isEnabled: Boolean | |
| get() = System.getProperty(PROPERTY_KEY) == "true" |
Comment on lines
+213
to
+252
| `when`("이미지 업로드 URL이 null이어도 리뷰 작성이 성공하면") { | ||
| val viewModel = WriteReviewViewModel(writeReviewUseCase, getImageUrlUseCase, getValidMenusOfMealUseCase) | ||
| val context = mockk<Context>() | ||
| val resolver = mockk<ContentResolver>() | ||
| val uri = mockk<Uri>() | ||
| val cacheDir = createTempDir(prefix = "write-review-null-url") | ||
| val compressed = File(cacheDir, "compressed.jpg").apply { writeBytes(byteArrayOf(1, 2, 3)) } | ||
|
|
||
| every { context.contentResolver } returns resolver | ||
| every { context.cacheDir } returns cacheDir | ||
| every { resolver.openInputStream(uri) } returns ByteArrayInputStream(byteArrayOf(1, 2, 3)) | ||
|
|
||
| mockkObject(Compressor) | ||
| coEvery { Compressor.compress(context, any()) } returns compressed | ||
| coEvery { getImageUrlUseCase(compressed) } returns null | ||
| coEvery { | ||
| writeReviewUseCase(MenuType.FIXED, 1L, 4, "", null, any()) | ||
| } returns true | ||
| mockkObject(EventLogger) | ||
| every { EventLogger.completeReview(any(), any(), any()) } just Runs | ||
|
|
||
| then("현재 동작대로 이미지 업로드 성공 토스트 후 리뷰 성공 흐름을 유지한다") { | ||
| runTest { | ||
| viewModel.loadMenuList(MenuType.FIXED, 1L, "돈가스") | ||
| advanceUntilIdle() | ||
| viewModel.onRatingChanged(4) | ||
| viewModel.setSelectedImage(uri) | ||
|
|
||
| viewModel.uiEvent.test { | ||
| viewModel.postReview(MenuType.FIXED, 1L, context) | ||
| advanceUntilIdle() | ||
|
|
||
| expectToast(R.string.toast_image_upload_success, ToastType.SUCCESS) | ||
| expectToast(R.string.toast_review_write_success, ToastType.SUCCESS) | ||
| expectNavigateBack() | ||
| cancelAndIgnoreRemainingEvents() | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
이미지 업로드에 실패하여 URL이 null로 반환되었음에도 '이미지 업로드 성공' 토스트가 표시되는 것은 사용자에게 혼란을 줄 수 있습니다. 이미지 URL을 받지 못한 경우는 실패로 간주하거나, 최소한 성공 토스트는 표시하지 않도록 수정하는 것이 좋겠습니다.
Comment on lines
+535
to
+585
| `when`("단과대만 바꾼 상태로 저장하면") { | ||
| val setUserNicknameUseCase = mockk<SetUserNicknameUseCase>() | ||
| val getUserCollegeDepartmentUseCase = mockk<GetUserCollegeDepartmentUseCase>() | ||
| val setUserCollegeDepartmentUseCase = mockk<SetUserCollegeDepartmentUseCase>() | ||
| val validateNicknameServerUseCase = mockk<ValidateNicknameServerUseCase>() | ||
| val validateNicknameLocalUseCase = mockk<ValidateNicknameLocalUseCase>() | ||
| val userRepository = mockk<UserRepository>() | ||
|
|
||
| coEvery { | ||
| getUserCollegeDepartmentUseCase() | ||
| } returns sampleUserInfo( | ||
| nickname = "oldNick", | ||
| college = baseCollege, | ||
| department = baseDepartment, | ||
| ) | ||
| coEvery { userRepository.getTotalColleges() } returns listOf(baseCollege, otherCollege) | ||
| coEvery { userRepository.getTotalDepartments(otherCollege.collegeId) } returns emptyList() | ||
| coEvery { userRepository.getTotalDepartments(baseCollege.collegeId) } returns listOf(baseDepartment) | ||
| every { validateNicknameLocalUseCase(any(), any(), any()) } returns NicknameValidationResult.Valid | ||
|
|
||
| val viewModel = UserInfoViewModel( | ||
| setUserNicknameUseCase = setUserNicknameUseCase, | ||
| getUserCollegeDepartmentUseCase = getUserCollegeDepartmentUseCase, | ||
| setUserCollegeDepartmentUseCase = setUserCollegeDepartmentUseCase, | ||
| validateNicknameServerUseCase = validateNicknameServerUseCase, | ||
| validateNicknameLocalUseCase = validateNicknameLocalUseCase, | ||
| userRepository = userRepository, | ||
| ) | ||
|
|
||
| then("현재 동작대로 Loading 상태에서 조기 종료된다") { | ||
| runTest { | ||
| eventually(2.seconds) { | ||
| (viewModel.uiState.value is UiState.Success) shouldBe true | ||
| } | ||
|
|
||
| viewModel.selectCollege(otherCollege) | ||
| advanceUntilIdle() | ||
|
|
||
| viewModel.uiEvent.test { | ||
| viewModel.saveUserInfo() | ||
| advanceUntilIdle() | ||
|
|
||
| expectNoEvents() | ||
| cancelAndIgnoreRemainingEvents() | ||
| } | ||
|
|
||
| viewModel.uiState.value shouldBe UiState.Loading | ||
| coVerify(exactly = 0) { userRepository.setUserDepartment(any()) } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
단과대만 변경하고 학과를 선택하지 않은 상태에서 저장을 시도하면 ViewModel이 Loading 상태에 멈추는 현상이 테스트를 통해 확인되었습니다. 이 경우, 저장 버튼을 비활성화하거나 사용자에게 학과를 선택해야 한다는 안내를 제공하여 UI가 멈추지 않도록 처리해야 합니다.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
이번 PR에서 Roborazzi + Robolectric 기반의 스크린샷 회귀 테스트 체계를 도입해
Activity/Fragment/Compose(Route/Screen) 단위의 화면 변화를 자동 검증하도록 구성했습니다.
핵심 효과:
ScreenCoverageGuardTest로 강제verifyRoborazziDebug로 회귀 자동 검증변경 규모:
Describe your changes
ScreenshotTestSeam+ 결정성 규칙으로 렌더 결과 고정:app:verifyRoborazziDebug추가주요 구현 내용
Gradle/의존성
recordRoborazzi/verifyRoborazzi실행 시에만 캡처되도록 시스템 프로퍼티 분기app/src/test/screenshots, 비교 산출물 경로를app/build/outputs/roborazzi로 정리결정성(Deterministic) 인프라
ScreenshotDeterminismRule로 Locale/Timezone/Animation scale 고정ScreenshotTestSeam추가커버리지 가드
ScreenCoverageRegistry,ScreenTargetScanner,ScreenCoverageGuardTest추가스크린샷 테스트 스위트
ActivityScreenSnapshotsTestFragmentScreenSnapshotsTestComposeRouteScreenshotsTestFakeUiStates,FakeScreenFixtures)로 loading/empty/success/error 케이스 캡처CI/문서
.github/workflows/debug.yml에 PR용screenshot-verifyjob 추가docs/SCREENSHOT_TESTING.md문서 추가 및 README 링크 연결Test
./gradlew :app:testDebugUnitTest --tests "com.eatssu.android.screenshot.inventory.ScreenCoverageGuardTest"✅./gradlew :app:verifyRoborazziDebug✅Issue
To reviewers
ScreenCoverageRegistry에 현재 대상 화면/상태가 충분히 반영됐는지ScreenshotTestSeam)이 프로덕션 동작에 영향 없이 테스트에서만 활성화되는지screenshot-verify실패 시 artifact 확인 흐름이 적절한지