-
Notifications
You must be signed in to change notification settings - Fork 21
refactor: UI Component Refactoring - ChatHeader.razor #452
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
refactor: UI Component Refactoring - ChatHeader.razor #452
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다! UI와 Logic이 잘 분리되었고,
추가해주신 테스트도 컨벤션을 잘 반영하고 있습니다.
[New Chat]
버튼이 Logic을 분리한 이후에도 잘 동작하는지
통합 테스트를 추가하면 좋겠습니다.
통합 테스트 시나리오는 다음과 같습니다.
아래와 같이 ChatHeader의 상위 컴포넌트에서 OnNewChat으로 메서드를 바인딩하고 있습니다.
<ChatHeader OnNewChat="@ResetConversationAsync" /> |
그 결과, OnNewChat 이벤트 콜백으로 아래 로직을 수행합니다.
open-chat-playground/src/OpenChat.PlaygroundApp/Components/Pages/Chat/Chat.razor.cs
Lines 70 to 76 in d650595
private async Task ResetConversationAsync() | |
{ | |
CancelAnyCurrentResponse(); | |
messages.Clear(); | |
messages.Add(new(ChatRole.System, SystemPrompt)); | |
await chatInput!.FocusAsync(); | |
} |
통합테스트 형식으로,
버튼을 누른 다음 위 로직을 실행했을때 기대되는 결과를 확인하면 될것 같아요.
[New Chat]
을 테스트하려면 일단 화면상에 채팅 버블이 표시되어 있어야하니,
테스트 카테고리는 통합테스트 + LLMRequired가 되겠네요.
음... 어떨까요? 이번 작업 범위는 벗어나는 테스트일까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다! 챗 UI에 어시스턴트 채팅 버블이 존재하는지 유무에 관계없이, UI를 완전히 초기화하는 방식이니까요. 굳이 세세하게 테스트 시나리오를 나누지 않아도 좋을 것 같습니다. 지금처럼 사용자 입장에서 의미있는 시나리오만 테스트해도 되겠네요.
Assert 부분에서 저희 컨벤션에 맞춰 Shouldly 사용하도록 변경하고 머지하면 될것 같아요.
test/OpenChat.PlaygroundApp.Tests/Components/Pages/Chat/ChatHeaderUITests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
민우님 ~ 아직 Expect가 남아있는 구문이 있습니다.
저는 테스트코드를 짤 때 링크에 있는 awesome copilot instruction을 수정해서 돌렸습니다.
저희팀 컨벤션을 반영해두었으니 한번 사용해보실래요?
#16 (comment)
test/OpenChat.PlaygroundApp.Tests/Components/Pages/Chat/ChatHeaderUITests.cs
Outdated
Show resolved
Hide resolved
test/OpenChat.PlaygroundApp.Tests/Components/Pages/Chat/ChatHeaderUITests.cs
Outdated
Show resolved
Hide resolved
참고해주신 이슈 잘 읽었습니다!! 감사합니다!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given/When/Then, Shoudly 사용 테스트 컨벤션을 잘 반영해주셨고,
회귀 테스트가 가능하게끔 통합테스트 시나리오도 추가해주셨습니다.
var noMessagesPlaceholder = Page.Locator(".no-messages");
을 사용해서,
LLM 응답이 올때까지 잘 대기합니다. 고생하셨습니다! 😎😎
Purpose
ChatHeader.razor
#299Does this introduce a breaking change?
Pull Request Type
What kind of change does this Pull Request introduce?
README updated?
The top-level readme for this repo contains a link to each sample in the repo. If you're adding a new sample did you update the readme?
How to Test
What to Check
Verify that the following are valid