Conversation
Updated the deployment workflow to use the control_dev image and added environment variables for the container.
📝 WalkthroughWalkthroughThis PR adds a comprehensive API integration test suite with a new GitHub Actions workflow to validate VM lifecycle operations. The changes introduce end-to-end tests for VM management, dependencies for mocking databases and Redis, and unit tests for SQL interactions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
structure/control_infra_sql_test.go (1)
29-36: Add one rollback-path test per transactional method.Both cases only assert the happy
BEGIN ... COMMITflow. IfAddInstanceorUpdateInstancestops rolling back after anExecfailure, this suite will still pass. A negative case that forces an error and expectsROLLBACKwould close that gap.Also applies to: 65-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@structure/control_infra_sql_test.go` around lines 29 - 36, The tests only cover the successful BEGIN...COMMIT path; add a negative test for each transactional method (AddInstance and UpdateInstance) that simulates an Exec failure and verifies a ROLLBACK is issued and the method returns an error. For each method, change the sqlmock setup to ExpectBegin(), set one of the Expected Exec calls (e.g., the inst_info or inst_loc insert used in the current diff) to WillReturnError(someErr), add mock.ExpectRollback(), call the method and assert it returns the error, and finally ensure mock.ExpectationsWereMet() passes so the rollback path is validated.api/vm_integration_test.go (1)
300-303: Use a bounded client in the HTTP helpers.Both helpers go through
http.DefaultClient, which has no timeout. If a handler or one of the mocked downstream calls hangs, this job waits for the full workflow timeout instead of failing fast.♻️ Proposed change
+var integrationHTTPClient = &http.Client{Timeout: 5 * time.Second} + func doJSONRequest(t *testing.T, srv *httptest.Server, method, path string, payload map[string]any) testHTTPResponse { t.Helper() @@ - resp, err := http.DefaultClient.Do(req) + resp, err := integrationHTTPClient.Do(req) @@ - resp, err := http.DefaultClient.Do(req) + resp, err := integrationHTTPClient.Do(req)Also applies to: 330-333
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/vm_integration_test.go` around lines 300 - 303, The test HTTP helpers currently call http.DefaultClient.Do(req) which has no timeout; replace those calls (both occurrences that match "http.DefaultClient.Do(req)" around the shown diff and the similar block at 330-333) with a bounded http.Client instance that sets a sensible Timeout (for example: client := &http.Client{Timeout: 5 * time.Second}; resp, err := client.Do(req)); alternatively, accept or reuse a shared *http.Client with a Timeout in the helper so requests fail fast instead of hanging..github/workflows/api-integration-tests.yaml (1)
31-34: The name-based-runfilter is easy to outgrow.This job only runs tests that keep the current
TestVM.../TestControlContext_...prefixes, so a renamed or newly added integration test in./apior./structurecan silently stop running while the workflow still stays green. Consider a dedicated build tag or package-level entrypoint instead of name-based selection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/api-integration-tests.yaml around lines 31 - 34, The workflow uses a fragile name-based filter "'TestVM.*|TestControlContext_.*'" in the go test invocation "go test -json -count=1 ./api ./structure -run 'TestVM.*|TestControlContext_.*'", which can silently drop renamed/new tests; instead mark integration tests with a dedicated build tag (e.g. add "//go:build integration" / "+build integration" to integration files or use a package-level TestMain) and update the workflow command to run "go test -json -count=1 -tags=integration ./api ./structure" (removing the -run filter) so tests are selected by tag rather than test-name prefixes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/vm_integration_test.go`:
- Around line 31-35: The mocked handlers for the POST routes "/BOOTVM" (case for
r.Method == http.MethodPost && r.URL.Path == "/BOOTVM") and "/forceShutDownUUID"
currently always return 200 without decoding/validating the JSON request body,
so add payload contract validation similar to the existing "/getStatusUUID"
branch: decode the request body into the expected struct (e.g., a UUID/payload
type used by getStatusUUID test), check for required fields/valid UUID format,
respond with 400 on decode/validation errors, and only write the 200 success
JSON when validation passes; update both BOOTVM and forceShutDownUUID handler
branches to mirror the validation logic used by getStatusUUID to ensure
malformed requests fail the test.
---
Nitpick comments:
In @.github/workflows/api-integration-tests.yaml:
- Around line 31-34: The workflow uses a fragile name-based filter
"'TestVM.*|TestControlContext_.*'" in the go test invocation "go test -json
-count=1 ./api ./structure -run 'TestVM.*|TestControlContext_.*'", which can
silently drop renamed/new tests; instead mark integration tests with a dedicated
build tag (e.g. add "//go:build integration" / "+build integration" to
integration files or use a package-level TestMain) and update the workflow
command to run "go test -json -count=1 -tags=integration ./api ./structure"
(removing the -run filter) so tests are selected by tag rather than test-name
prefixes.
In `@api/vm_integration_test.go`:
- Around line 300-303: The test HTTP helpers currently call
http.DefaultClient.Do(req) which has no timeout; replace those calls (both
occurrences that match "http.DefaultClient.Do(req)" around the shown diff and
the similar block at 330-333) with a bounded http.Client instance that sets a
sensible Timeout (for example: client := &http.Client{Timeout: 5 * time.Second};
resp, err := client.Do(req)); alternatively, accept or reuse a shared
*http.Client with a Timeout in the helper so requests fail fast instead of
hanging.
In `@structure/control_infra_sql_test.go`:
- Around line 29-36: The tests only cover the successful BEGIN...COMMIT path;
add a negative test for each transactional method (AddInstance and
UpdateInstance) that simulates an Exec failure and verifies a ROLLBACK is issued
and the method returns an error. For each method, change the sqlmock setup to
ExpectBegin(), set one of the Expected Exec calls (e.g., the inst_info or
inst_loc insert used in the current diff) to WillReturnError(someErr), add
mock.ExpectRollback(), call the method and assert it returns the error, and
finally ensure mock.ExpectationsWereMet() passes so the rollback path is
validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b60314f2-51fd-49de-85cc-f652044ea5a4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
.github/workflows/api-integration-tests.yamlapi/vm_integration_test.gogo.modstructure/control_infra_sql_test.go
| case r.Method == http.MethodPost && r.URL.Path == "/BOOTVM": | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusOK) | ||
| _, _ = w.Write([]byte(`{"message":"BootVM operation success"}`)) | ||
| return |
There was a problem hiding this comment.
Validate the mocked core payloads for start and shutdown.
Those branches always return 200 without decoding the request body, so this test will not catch a broken UUID or payload shape for /BOOTVM or /forceShutDownUUID. The /getStatusUUID branch already does contract validation; mirror that here so all lifecycle calls are actually verified.
Also applies to: 51-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/vm_integration_test.go` around lines 31 - 35, The mocked handlers for the
POST routes "/BOOTVM" (case for r.Method == http.MethodPost && r.URL.Path ==
"/BOOTVM") and "/forceShutDownUUID" currently always return 200 without
decoding/validating the JSON request body, so add payload contract validation
similar to the existing "/getStatusUUID" branch: decode the request body into
the expected struct (e.g., a UUID/payload type used by getStatusUUID test),
check for required fields/valid UUID format, respond with 400 on
decode/validation errors, and only write the 200 success JSON when validation
passes; update both BOOTVM and forceShutDownUUID handler branches to mirror the
validation logic used by getStatusUUID to ensure malformed requests fail the
test.
@ga111o |
kwonkwonn
left a comment
kwonkwonn
left a comment
There was a problem hiding this comment.
아직 마이그레이션이 되지 않은 부분이 너무 많기도 하고 유닛테스트도 없어 통합테스트를 바로 붙이기엔 어렵지 않을까 생각합니다
깃허브 워크플로우자체는 좋아보입니다...
| } | ||
| } | ||
|
|
||
| type testHTTPResponse struct { |
There was a problem hiding this comment.
테스트 유틸함수는 밖으로 빼거나 위로 올리는게 좀 더 읽기 쉬울 것 같습니다.
또 통합테스트라면 /test 로 빼는게 좀 더 이쁠 거 같아요.
There was a problem hiding this comment.
통합테스트가 아니라, 예상 값들 바탕으로 api직접 값 넣어보는 유닛테스트에 가까워 괜찮지 않을까 싶었습니다.
테스트 유틸함수는 확실히 위로 올리는 게 더 좋을 것 같네요.
통합 테스트이면 /test로 빼는 게 좋아보이긴 하는데, api 요청/응답 부분만 각각 테스트하는 코드라 api폴더에 넣어놓긴 했습니다.
There was a problem hiding this comment.
아하 테스트 이름 보고 헷갈렸네요.
유닛테스트는 보통 "*_test.go" 로 파일별로 생성하는 것을 컨밴션으로 알고 있는데 한번 확인해주세용.(go test가 자동으로 인식할겁니다)
파일이 너무 많으면 오히려 가독성이 떨어질 것 같아서 그런경우에는 그냥 지금처럼 이름지어도 될 거 같긴합니다.
테스트 코드중에서 api 레이어 전반에서 사용할만한 함수가 있다면 /test/api로 빼고 가져다 쓰는것도 방법이 될 수 있을 것 같습니다.
|
|
|
#37 테스트 -> 마이그레이션이 되면 2중으로 마이그레이션을 진행해야 돼서 작업이 복잡해질 것 같습니다. http + my_sql 모킹 및 검증으로 구성되어 있는 것 같은데 해당 함수들을 |
.github의 workflow는 어느 브랜치에 있던 해당 브랜치에서는 정상작동하긴 합니다. 하지만 actions 탭에서 toggle 등을 통해 직접 실행시키고 관리하려면 main에 있어야 적용됩니다. |
|
|
||
| jobs: | ||
| api-integration-tests: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
self hosted runner를 사용하지 않는 이유가 있을까요?
There was a problem hiding this comment.
테스트 돌리는 서버는 실제 서버와 분리시키는 게 좋다고 생각했습니다
컴퓨팅 자원 많이 먹거나 하는 테스트가 아녀서 별도 ci서버 들이기 전까지는 저렇게 해도 무방하지 않을까 싶었습니다.
3 participants
Summary by CodeRabbit
Release Notes
Tests
Chores