Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: 동시성 테스트 추가 #604

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Conversation

SuyeonChoi
Copy link
Collaborator

구현 기능

  • 동시성 문제가 발생할 수 있는 기능에 대해 테스트를 추가했습니다.
    • 레벨로그 생성
    • 멤버 생성
    • 피드백 생성
    • 인터뷰질문 좋아요

기타

동시성 이슈를 테스트 코드로 검증할 방법이 떠오르지 않아서 테스트는 없습니당 (ㅠㅡㅠ)

@SuyeonChoi SuyeonChoi self-assigned this Nov 27, 2022
@github-actions
Copy link

Unit Test Results

429 tests   429 ✔️  25s ⏱️
126 suites      0 💤
126 files        0

Results for commit 2d3dec0.

@github-actions
Copy link

🏃‍♀️💨 Code Analysis Results By SonarQube

levellog-604

Bugs : 0 ✅
Vulnerabilities : 0 ✅
Code Smells : 0 ✅
Coverage : 99.1 ✅

리포트 확인하기

Copy link
Collaborator

@nailseong nailseong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 괴퍼 (괴물 개발자 페퍼)
주말인데도 고생하셨네요. 동시성 테스트를 생각 못하고 있었는데 쓰레기 찾아서 주워주셨네요 🐶
저는 지금도 괜찮은 테스트 방법이라고 생각해서 좋다고 생각해요.

소소한 코멘트 남겨봤는데 심심할 때 확인 한 번 해주세요 ^~^
우가우가 🍗

Comment on lines +26 to +30
if (response.isSuccess()) {
successRequest.incrementAndGet();
} else {
failRequest.incrementAndGet();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

질문

페퍼가 생각한 성공/실패200과 200이 아닌 응답 인가요? 코드로 보면 그렇다고 생각이 드는데요.
작성해 주신 동시성 테스트를 통해서 검증하고 싶은것이 잘 이루어지고 있는지 궁금해요 ^~^

제가 그렇게 생각한 이유를 말씀 드릴게요.
위 코드를 아래와 같이 5XX 응답만 카운트 하도록 변경하고 테스트를 실행해 봤어요.

if (response.isSuccess()) {
    successRequest.incrementAndGet();
}
if (response.isInternalServerError()) {
    failRequest.incrementAndGet();
}

그랬더니 아래와 같이 회원가입 요청 테스트에서 400응답이 떨이지고 테스트에 실패했어요.
스크린샷 2022-11-27 오후 9 44 28

즉, 지금의 동시성 테스트 코드에 검증이 동시 요청에 대한 검증이 아닐 수도 있다.라는 생각이 듭니다.
단순히 2XX이 아닌 응답을 모두 failRequest로 카운트하면 정말 동시 요청이 날라가고 그에 대한 검증이 이루어진걸까..? 라는 생각이 들더라구요.

🌟물론!!!🌟 어떤 응답이 내려가든 1개의 요청만 성공해야 한다가 페퍼가 검증하고 싶은 내용이라면 지금도 충분하다고 생각합니다. ^ㅠ^

Comment on lines +32 to +35
} catch (final Exception e) {
failRequest.incrementAndGet();
throw new RuntimeException(e);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try-catch로 감싼 이유는 무엇인가요?!

Comment on lines +29 to +37
// given
final int threadPoolSize = 2;
final ConcurrentRequester concurrentRequester = new ConcurrentRequester(threadPoolSize);

// when
for (int i = 0; i < threadPoolSize; i++) {
concurrentRequester.execute(PEPPER::login);
}
concurrentRequester.await();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// given
final int threadPoolSize = 2;
final ConcurrentRequester concurrentRequester = new ConcurrentRequester(threadPoolSize);
// when
for (int i = 0; i < threadPoolSize; i++) {
concurrentRequester.execute(PEPPER::login);
}
concurrentRequester.await();
// given
final int requestCount = 2;
final ConcurrentRequester concurrentRequester = new ConcurrentRequester();
// when
concurrentRequester.execute(PEPPER::login, requestCount);

ConcurrentRequester.execute 내부에서 countDownLatch.await()까지 실행하면 어떨까요? 반복문 코드도 내부에 감춘다면 사용성이 더 좋을 것 같다고 생각해요 🥺

Comment on lines +40 to +42
final int numberOfSavedMember = getResponse("/api/members?nickname=페퍼", MemberResponses.class)
.getMembers()
.size();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a. concurrentRequester의 successRequest로 검증이 충분하다고 생각하는데 페퍼는 어떻게 생각하세요..?

Comment on lines +18 to +19
@DisplayName("동시성 관련 기능")
class ConcurrentAcceptanceTest extends AcceptanceTest {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r. 사전질문 생성도 테스트가 필요하다고 생각해요 ^~^

Copy link
Collaborator

@2yujeong 2yujeong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 페퍼!
동시성 테스트는 직접 작성해본 적이 없어서 잘 몰랐는데 ExecutorService, CountDownLatch, AtomicInteger 잘 활용해주셨네요👍🏼
현재 상황에서는 우리가 기대하는 동시성 문제 발생 상황을 확실하게 테스트할 수 있는 방법이 떠오르지 않아서 지금 테스트도 충분히 좋다고 생각해요.
고생하셨습니다🙂

+ 동시성 테스트를 서비스 테스트가 아닌 인수 테스트로 작성하신 이유가 궁금해요

Comment on lines +23 to +24
* when: 동시에 여러 개의 피드백 작성을 요청한다.
* then: 한 개의 피드백만 작성에 성공한다.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when, then 수정이 필요해 보여용

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants