254 lines
10 KiB
Text
254 lines
10 KiB
Text
<!-- task=kotlin_impl plan=1 tag=REVIEW_API -->
|
|
|
|
# Code Review Reference - REVIEW_API
|
|
|
|
## 개요
|
|
|
|
task=kotlin_impl, plan=1, tag=REVIEW_API
|
|
|
|
## 구현 항목별 완료 여부
|
|
|
|
| 항목 | 완료 여부 |
|
|
|------|---------|
|
|
| [REVIEW_API-1] `WsServer.stop()` `InterruptedException` 미처리 수정 | [x] |
|
|
| [REVIEW_API-2] `crosstest/` 를 main sourceset에서 분리 | [x] |
|
|
| [REVIEW_API-3] `kotlin_go.kt` listener 내 `runBlocking` 제거 | [x] |
|
|
| [REVIEW_API-4] `HeartbeatTest.testHeartbeatTimerResetOnReceivedData` 테스트 추가 | [x] |
|
|
| [REVIEW_API-5] `Communicator.parse()` visibility `internal` 로 제한 | [x] |
|
|
|
|
## 계획 대비 변경 사항
|
|
|
|
- REVIEW_API-3의 listener send는 계획의 `client.scope.launch` 대신 `runTcpSendPush`/`runWsSendPush`를 `coroutineScope`로 감싸고 해당 scope의 `launch`를 사용했다. `crosstest`를 main sourceset에서 분리하면 main의 `internal` 멤버 접근이 별도 compilation에서 막힐 수 있어, crosstest runner가 main 내부 scope에 의존하지 않도록 했다.
|
|
- REVIEW_API-3 구현 중 `WsClient.forServer`도 `internal`이면 분리된 crosstest sourceset에서 `kotlin_go.kt`가 접근할 수 없다. 또한 public `WsServer` 생성자에서 기본 server-side `WsClient`를 만들 방법이 필요하므로 `WsClient.forServer`를 public companion factory로 조정했다.
|
|
- REVIEW_API-3 검증 중 분리된 crosstest sourceset에서 `parserMap()`의 `TestData.parseFrom(it)` overload 추론이 모호해져 `parserMap(): ParserMap` 반환 타입을 명시했다.
|
|
- REVIEW_API-4의 수신 경로 테스트는 `TestData` listener를 등록하고 `received` 플래그를 확인해 `onReceivedData()`가 실제 listener dispatch까지 통과했는지도 검증한다.
|
|
- 최종 검증 중 `TcpTest` class 실행이 timeout 된 뒤 메서드 단위로 분리해 확인했다. `testTcpClientCloseIdempotent`의 blocking `ServerSocket.accept()`를 `Dispatchers.IO`에서 실행하도록 바꿔 테스트 스레드 점유 위험을 낮췄고, 이후 `TcpTest`, `WsTest`, 전체 `./gradlew test`가 통과했다.
|
|
|
|
## 주요 설계 결정
|
|
|
|
- `crosstest` sourceset은 main 출력과 runtime classpath를 compile/runtime classpath로 갖도록 구성했다.
|
|
- `run` task는 `crosstest` runtime classpath에서 기본 crosstest main class를 실행하도록 재구성했다.
|
|
- `BaseClient.scope`는 계획대로 `internal`로 제한했지만, crosstest runner는 이 scope를 직접 참조하지 않는다.
|
|
- Gradle 검증은 `JAVA_HOME=/config/opt/jdk/jdk-17.0.10+7`, `GRADLE_USER_HOME=/tmp/gradle` 환경으로 실행했다.
|
|
|
|
## 리뷰어를 위한 체크포인트
|
|
|
|
- `crosstest` sourceset 분리 후 `./gradlew run` 이 여전히 `MainKt` 를 실행하는가?
|
|
- `BaseClient.scope` 를 `internal` 로 노출 시 외부 모듈에서 scope를 직접 조작하는 위험이 없는가?
|
|
- `testHeartbeatTimerResetOnReceivedData` 가 `onReceivedData` 경로를 실제로 통과하는가?
|
|
- JAR 에 crosstest 클래스가 포함되지 않는가?
|
|
|
|
## 검증 결과
|
|
|
|
_구현 에이전트가 각 중간 검증 및 최종 검증 명령 실행 후 출력을 여기에 붙여 넣는다._
|
|
|
|
### REVIEW_API-1 중간 검증
|
|
```
|
|
$ ./gradlew compileKotlin
|
|
> Task :checkKotlinGradlePluginConfigurationErrors SKIPPED
|
|
> Task :extractIncludeProto UP-TO-DATE
|
|
> Task :extractProto UP-TO-DATE
|
|
> Task :generateProto UP-TO-DATE
|
|
> Task :compileKotlin UP-TO-DATE
|
|
|
|
BUILD SUCCESSFUL in 13s
|
|
4 actionable tasks: 4 up-to-date
|
|
```
|
|
|
|
### REVIEW_API-2 중간 검증
|
|
```
|
|
$ ./gradlew compileKotlin compileCrosstestKotlin
|
|
> Task :checkKotlinGradlePluginConfigurationErrors SKIPPED
|
|
> Task :extractIncludeProto UP-TO-DATE
|
|
> Task :extractProto UP-TO-DATE
|
|
> Task :generateProto UP-TO-DATE
|
|
> Task :compileKotlin UP-TO-DATE
|
|
> Task :compileJava UP-TO-DATE
|
|
> Task :processResources UP-TO-DATE
|
|
> Task :classes UP-TO-DATE
|
|
> Task :extractCrosstestProto UP-TO-DATE
|
|
> Task :extractIncludeCrosstestProto UP-TO-DATE
|
|
> Task :generateCrosstestProto NO-SOURCE
|
|
> Task :compileCrosstestKotlin
|
|
|
|
BUILD SUCCESSFUL in 11s
|
|
9 actionable tasks: 1 executed, 8 up-to-date
|
|
|
|
$ ./gradlew jar
|
|
> Task :checkKotlinGradlePluginConfigurationErrors SKIPPED
|
|
> Task :extractIncludeProto UP-TO-DATE
|
|
> Task :extractProto UP-TO-DATE
|
|
> Task :generateProto UP-TO-DATE
|
|
> Task :compileKotlin UP-TO-DATE
|
|
> Task :compileJava UP-TO-DATE
|
|
> Task :processResources UP-TO-DATE
|
|
> Task :classes UP-TO-DATE
|
|
> Task :jar
|
|
|
|
BUILD SUCCESSFUL in 3s
|
|
7 actionable tasks: 1 executed, 6 up-to-date
|
|
|
|
$ jar tf build/libs/toki-socket-kotlin-0.1.0.jar | grep crosstest
|
|
(no output)
|
|
```
|
|
|
|
### REVIEW_API-3 중간 검증
|
|
```
|
|
$ ./gradlew compileCrosstestKotlin
|
|
> Task :checkKotlinGradlePluginConfigurationErrors SKIPPED
|
|
> Task :extractIncludeProto UP-TO-DATE
|
|
> Task :extractProto UP-TO-DATE
|
|
> Task :generateProto UP-TO-DATE
|
|
> Task :compileKotlin UP-TO-DATE
|
|
> Task :compileJava UP-TO-DATE
|
|
> Task :processResources UP-TO-DATE
|
|
> Task :classes UP-TO-DATE
|
|
> Task :extractCrosstestProto UP-TO-DATE
|
|
> Task :extractIncludeCrosstestProto UP-TO-DATE
|
|
> Task :generateCrosstestProto NO-SOURCE
|
|
> Task :compileCrosstestKotlin UP-TO-DATE
|
|
|
|
BUILD SUCCESSFUL in 3s
|
|
9 actionable tasks: 9 up-to-date
|
|
```
|
|
|
|
### REVIEW_API-4 중간 검증
|
|
```
|
|
$ ./gradlew test --tests "*.HeartbeatTest"
|
|
> Task :checkKotlinGradlePluginConfigurationErrors SKIPPED
|
|
> Task :extractIncludeProto UP-TO-DATE
|
|
> Task :extractProto UP-TO-DATE
|
|
> Task :generateProto UP-TO-DATE
|
|
> Task :compileKotlin UP-TO-DATE
|
|
> Task :compileJava UP-TO-DATE
|
|
> Task :processResources UP-TO-DATE
|
|
> Task :classes UP-TO-DATE
|
|
> Task :extractIncludeTestProto UP-TO-DATE
|
|
> Task :extractTestProto UP-TO-DATE
|
|
> Task :generateTestProto NO-SOURCE
|
|
> Task :compileTestKotlin UP-TO-DATE
|
|
> Task :compileTestJava NO-SOURCE
|
|
> Task :processTestResources NO-SOURCE
|
|
> Task :testClasses UP-TO-DATE
|
|
> Task :test
|
|
|
|
BUILD SUCCESSFUL in 15s
|
|
10 actionable tasks: 1 executed, 9 up-to-date
|
|
```
|
|
|
|
### REVIEW_API-5 중간 검증
|
|
```
|
|
$ ./gradlew compileKotlin compileTestKotlin
|
|
> Task :checkKotlinGradlePluginConfigurationErrors SKIPPED
|
|
> Task :extractIncludeProto UP-TO-DATE
|
|
> Task :extractProto UP-TO-DATE
|
|
> Task :generateProto UP-TO-DATE
|
|
> Task :compileKotlin UP-TO-DATE
|
|
> Task :compileJava UP-TO-DATE
|
|
> Task :processResources UP-TO-DATE
|
|
> Task :classes UP-TO-DATE
|
|
> Task :extractIncludeTestProto UP-TO-DATE
|
|
> Task :extractTestProto UP-TO-DATE
|
|
> Task :generateTestProto NO-SOURCE
|
|
> Task :compileTestKotlin
|
|
|
|
BUILD SUCCESSFUL in 8s
|
|
9 actionable tasks: 1 executed, 8 up-to-date
|
|
```
|
|
|
|
### 최종 검증
|
|
```
|
|
$ ./gradlew test
|
|
> Task :checkKotlinGradlePluginConfigurationErrors SKIPPED
|
|
> Task :extractIncludeProto UP-TO-DATE
|
|
> Task :extractProto UP-TO-DATE
|
|
> Task :generateProto UP-TO-DATE
|
|
> Task :compileKotlin UP-TO-DATE
|
|
> Task :compileJava UP-TO-DATE
|
|
> Task :processResources UP-TO-DATE
|
|
> Task :classes UP-TO-DATE
|
|
> Task :extractIncludeTestProto UP-TO-DATE
|
|
> Task :extractTestProto UP-TO-DATE
|
|
> Task :generateTestProto NO-SOURCE
|
|
> Task :compileTestKotlin UP-TO-DATE
|
|
> Task :compileTestJava NO-SOURCE
|
|
> Task :processTestResources NO-SOURCE
|
|
> Task :testClasses UP-TO-DATE
|
|
> Task :test
|
|
|
|
BUILD SUCCESSFUL in 16s
|
|
10 actionable tasks: 1 executed, 9 up-to-date
|
|
|
|
$ ./gradlew jar
|
|
> Task :checkKotlinGradlePluginConfigurationErrors SKIPPED
|
|
> Task :extractIncludeProto UP-TO-DATE
|
|
> Task :extractProto UP-TO-DATE
|
|
> Task :generateProto UP-TO-DATE
|
|
> Task :compileKotlin UP-TO-DATE
|
|
> Task :compileJava UP-TO-DATE
|
|
> Task :processResources UP-TO-DATE
|
|
> Task :classes UP-TO-DATE
|
|
> Task :jar
|
|
|
|
BUILD SUCCESSFUL in 3s
|
|
7 actionable tasks: 1 executed, 6 up-to-date
|
|
|
|
$ jar tf build/libs/toki-socket-kotlin-0.1.0.jar | grep crosstest
|
|
(no output)
|
|
|
|
$ /usr/bin/bash -lc 'PATH=/config/go-sdk/go/bin:/config/go/bin:$PATH GOCACHE=/tmp/go-build GOMODCACHE=/tmp/go-mod go test ./...'
|
|
? toki-labs.com/toki_socket/go [no test files]
|
|
? toki-labs.com/toki_socket/go/crosstest/dart_go_client [no test files]
|
|
? toki-labs.com/toki_socket/go/crosstest/kotlin_go_client [no test files]
|
|
? toki-labs.com/toki_socket/go/examples/tcp_echo [no test files]
|
|
? toki-labs.com/toki_socket/go/examples/ws_echo [no test files]
|
|
? toki-labs.com/toki_socket/go/packets [no test files]
|
|
ok toki-labs.com/toki_socket/go/test (cached)
|
|
|
|
$ bash tools/check_proto_sync.sh
|
|
Proto schemas are in sync.
|
|
```
|
|
|
|
---
|
|
|
|
## 코드리뷰 결과
|
|
|
|
### 종합 판정
|
|
|
|
**PASS**
|
|
|
|
---
|
|
|
|
### 차원별 평가
|
|
|
|
| 차원 | 판정 | 비고 |
|
|
|------|------|------|
|
|
| 정확성 (Correctness) | Pass | REVIEW_API-1~5 모두 의도대로 구현됨. 멱등성, race 보호, crosstest 분리 정상 |
|
|
| 완성도 (Completeness) | Pass | 5개 항목 전부 구현. `./gradlew test`, `jar`, Go 테스트, proto sync 통과 |
|
|
| 테스트 커버리지 (Test coverage) | Pass | `testHeartbeatTimerResetOnReceivedData` 추가. listener dispatch 경로 검증 포함 |
|
|
| API 계약 (API contract) | Pass | crosstest가 JAR에서 제외됨 확인. `parse()` internal 제한 |
|
|
| 코드 품질 (Code quality) | Pass | `runBlocking` in listener 제거, `InterruptedException` 처리 완료 |
|
|
| 계획 대비 변경 (Plan deviation) | Pass | 모든 변경이 CODE_REVIEW.md에 이유와 함께 기록됨 |
|
|
| 검증 신뢰도 (Verification trust) | Pass | JVM 환경에서 전 항목 실제 실행 확인 |
|
|
|
|
---
|
|
|
|
### 발견된 문제
|
|
|
|
**Nit — `testHeartbeatTimerResetOnReceivedData` 테스트 이름과 검증 범위 불일치**
|
|
`kotlin/src/test/kotlin/com/tokilabs/toki_socket/HeartbeatTest.kt:84-106`
|
|
|
|
테스트 이름은 "수신 데이터에 의한 타이머 리셋"이지만, 타이머를 실제로 리셋하는 것은 `onReceivedData` 이후 명시적으로 호출하는 `sendHeartBeat()`이다. `onReceivedData` 자체가 타이머를 리셋하는 경로(`TcpClient.readLoop`, `WsClient.receiveBytes`에서 `sendHeartBeat()` 호출)는 `HeartbeatClient`에서 재현되지 않는다. `received` 플래그 확인으로 listener dispatch는 검증되나, 테스트 이름이 과장된다. 블로킹 이슈는 아님.
|
|
|
|
---
|
|
|
|
**Info — `build.gradle.kts` `application.mainClass` 중복 설정**
|
|
`kotlin/build.gradle.kts:46-60`
|
|
|
|
`application { mainClass }` 와 `tasks.named<JavaExec>("run") { mainClass }` 에 동일한 값이 중복 설정되어 있다. `tasks.named("run")`이 오버라이드하므로 `application { }` 블록의 설정은 실질적으로 무효이다. 기능상 문제는 없으나 `application { mainClass }` 제거 또는 주석 추가가 명확하다.
|
|
|
|
---
|
|
|
|
### 다음 단계
|
|
|
|
PASS: 후속 PLAN.md 없음. kotlin_impl 완료.
|