integrate test - #28
swthewhite wants to merge 4 commits into
Conversation
Summary of ChangesHello @swthewhite, 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! This pull request significantly enhances the project's testing framework and database schema. It integrates industry-standard tools like JaCoCo and SonarQube to improve code quality and coverage reporting. The testing environment has been upgraded by migrating to Testcontainers for MongoDB, ensuring more robust and isolated integration tests. Furthermore, new dedicated integration tests have been added for key data repositories, and the database schema has been refined with improved data types, constraints, and indexing for better consistency and performance. Highlights
Ignored Files
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}
There was a problem hiding this comment.
Code Review
이 PR은 통합 테스트 환경을 구축하고, Jacoco와 SonarQube를 연동하여 코드 커버리지 및 정적 분석을 설정하는 변경 사항을 포함하고 있습니다. 또한 ProblemJdbcRepository와 UserJdbcRepository에 대한 테스트 코드를 추가하고, 관련 데이터베이스 스키마를 개선했습니다.
전반적으로 좋은 방향의 변경이지만, 몇 가지 개선점을 제안합니다.
build.gradle의 SonarQube 설정에서 환경 변수가 없을 경우 빌드가 명시적으로 실패하도록 하여 설정 오류를 방지하는 것이 좋습니다.- 새로 추가된 테스트 코드에서
LocalDateTime.now()사용으로 인한 테스트 불안정성 문제, 불필요한 코드, 타입 불일치 등의 문제를 발견했습니다. - 코드 가독성 및 유지보수성 향상을 위한 제안도 포함되어 있습니다.
자세한 내용은 각 파일의 인라인 코멘트를 참고해주세요.
| String fetchUsersTier=jdbcTemplate.queryForObject(sql,String.class,bojId); | ||
|
|
||
| assertThat(fetchUsersTier).isEqualTo(String.valueOf(newTier)); |
There was a problem hiding this comment.
boj_tier 컬럼은 init_schema.sql에 integer 타입으로 정의되어 있습니다. 하지만 테스트 코드에서는 String.class로 조회하고 있습니다. 이는 JDBC 드라이버의 암묵적 형변환에 의존하는 불안정한 방식입니다. 데이터베이스 스키마와 일치하는 Integer.class로 조회하고, int 타입과 직접 비교하여 테스트의 정확성과 안정성을 높이는 것이 좋습니다.
Integer fetchUsersTier = jdbcTemplate.queryForObject(sql, Integer.class, bojId);
assertThat(fetchUsersTier).isEqualTo(newTier);| property 'sonar.organization', System.getenv('SONAR_ORGANIZATION') ?: '' | ||
| property 'sonar.projectKey', System.getenv('SONAR_PROJECT') ?: '' |
There was a problem hiding this comment.
환경 변수 SONAR_ORGANIZATION와 SONAR_PROJECT가 설정되지 않았을 때 빈 문자열('')을 기본값으로 사용하고 있습니다. 이 경우 SonarQube 분석이 조용히 실패하거나 잘못된 프로젝트로 리포팅될 수 있습니다. ?: '' 부분을 제거하여 환경 변수가 없을 때 null이 전달되도록 하면, SonarQube 플러그인이 이를 감지하고 빌드를 명시적으로 실패시킬 가능성이 높습니다. 이는 설정 오류를 더 빨리 발견하는 데 도움이 됩니다.
property 'sonar.organization', System.getenv('SONAR_ORGANIZATION')
property 'sonar.projectKey', System.getenv('SONAR_PROJECT')
| //제약조건 | ||
| Integer count = jdbcTemplate.queryForObject(""" | ||
| SELECT COUNT(*) | ||
| FROM pg_constraint | ||
| WHERE conrelid = 'users_problem'::regclass | ||
| AND conname = 'users_problem_unique' | ||
| """, Integer.class); | ||
|
|
||
| // 없으면 추가 | ||
| if (count != null && count.equals(0)) { | ||
| jdbcTemplate.execute(""" | ||
| ALTER TABLE users_problem | ||
| ADD CONSTRAINT users_problem_unique UNIQUE (user_id, problem_id) | ||
| """); | ||
| } |
There was a problem hiding this comment.
@BeforeEach 메서드 내에서 데이터베이스 제약 조건을 프로그래밍 방식으로 확인하고 추가하는 로직이 있습니다. init_schema.sql 파일에 이미 제약 조건이 정의되어 있으므로 이 코드는 불필요합니다. 테스트 설정의 단일 책임 원칙을 지키고 init_schema.sql을 스키마의 유일한 소스로 유지하기 위해 이 부분을 제거하는 것을 권장합니다. 이렇게 하면 테스트 코드가 더 깔끔해지고 스키마 관리가 용이해집니다.
| //given | ||
| Long userId = 12L; | ||
| Integer problemId = 1000; | ||
| LocalDateTime batchTime = LocalDateTime.now(); |
There was a problem hiding this comment.
LocalDateTime.now()를 사용하면 데이터베이스의 타임스탬프 정밀도 차이로 인해 테스트가 간헐적으로 실패할 수 있습니다. 테스트의 안정성과 재현성을 위해 고정된 시간 값을 사용하는 것이 좋습니다.
| LocalDateTime batchTime = LocalDateTime.now(); | |
| LocalDateTime batchTime = LocalDateTime.of(2024, 1, 1, 0, 0); |
| Long userId = 12L; | ||
| Integer problemId = 1001; | ||
| LocalDateTime epochTime = LocalDateTime.of(1970, 1, 1, 0, 0, 0); | ||
| LocalDateTime batchTime = LocalDateTime.now(); |
There was a problem hiding this comment.
LocalDateTime.now()를 사용하면 데이터베이스의 타임스탬프 정밀도 차이로 인해 테스트가 간헐적으로 실패할 수 있습니다. 테스트의 안정성과 재현성을 위해 고정된 시간 값을 사용하는 것이 좋습니다.
| LocalDateTime batchTime = LocalDateTime.now(); | |
| LocalDateTime batchTime = LocalDateTime.of(2024, 1, 1, 0, 0); |
| List<String> bojIds=new ArrayList<>(); | ||
| for (UserInfo info:results){ | ||
| bojIds.add(info.baekJoonId()); | ||
| } | ||
| assertThat(bojIds).containsExactlyInAnyOrder("test1","test2","test3","test4"); |
There was a problem hiding this comment.
결과 리스트에서 baekJoonId를 추출하여 검증하는 로직을 AssertJ의 extracting과 containsExactlyInAnyOrder를 사용하여 더 간결하게 표현할 수 있습니다. 이렇게 하면 코드가 더 읽기 쉬워지고 테스트의 의도가 명확해집니다.
assertThat(results).extracting(UserInfo::baekJoonId)
.containsExactlyInAnyOrder("test1", "test2", "test3", "test4");
b217847 to
c03113e
Compare
c03113e to
8d9264e
Compare
1 participant
🍀 이슈 번호
✅ 작업 사항
⌨ 기타