Feature/235 connector implementation inheritance upstage #463

gremh97 · 2025-09-23T13:10:53Z

Purpose

Implements the Upstage connector using the OpenAI SDK, enabling support for Upstage AI models in the playground.
Adds comprehensive unit tests for configuration validation, error handling, and ChatClient creation.
Improves configuration management with clear priority: CLI arguments > environment variables > appsettings.json.
Updates documentation to include architecture analysis and implementation guide for the new connector

Does this introduce a breaking change?

[ ] Yes
[x] No

Pull Request Type

What kind of change does this Pull Request introduce?

[ ] Bugfix
[x] New feature
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

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?

[ ] Yes
[x] No
[ ] N/A

How to Test

Get the code

git clone https://github.com/gremh97/open-chat-playground.git
cd open-chat-playground
git checkout feature/235-connector-implementation-inheritance-upstage

Test the code

# Build and run the application
dotnet build
dotnet run --project src/OpenChat.PlaygroundApp -- --help

# Run Upstage-specific tests
dotnet test --filter "Upstage" --verbosity normal

# Run Upstage connector tests
dotnet test --filter "FullyQualifiedName~UpstageConnectorTests" --verbosity normal

What to Check

Verify that the following are valid
All unit tests pass successfully
Application builds without errors
Upstage connector works as expected with CLI and environment variable configuration
No regression in existing features
Code follows project conventions and standards
Documentation is clear and accurate

Other Information

Include any additional context about the implementation
Mention any dependencies or prerequisites
Note any breaking changes or migration steps required
Reference related issues or discussions

Resolves #235

- Add UpstageConnector class inheriting from LanguageModelConnector - Implement EnsureLanguageModelSettingsValid() with BaseUrl, ApiKey, Model validation - Implement GetChatClientAsync() using OpenAI SDK with custom endpoint for Upstage API - Add Upstage case to LanguageModelConnector.CreateChatClientAsync() factory method - Leverage OpenAI SDK compatibility since Upstage API follows OpenAI format - Enable connection to Upstage Solar AI models through the application Partially resolves aliencube#235

…ance-upstage

- Added test/OpenChat.PlaygroundApp.Tests/Connectors/UpstageConnectorTests.cs - Covers configuration validation, error handling, and ChatClient creation for Upstage connector - Ensures reliability and correct behavior for Upstage integration

sikutisa

일단 Connector와 테스트 코드 먼저 리뷰하겠습니다.
테스트가 더 추가돼야 합니다.

UpstageConnector가 LanguageModelConnector를 상속하는지에 대한 테스트
테스트해야 하는 메소드는 총 3개입니다.
- EnsureLanguageModelSettingsValid()
- GetChatClientAsync()
- LanguageModelConnector.CreateChatClientAsync()

이 내용 한 번 봐보시죠.
#447 (comment)

test/OpenChat.PlaygroundApp.Tests/Connectors/UpstageConnectorTests.cs

…rTests - Add private const fields for BaseUrl, ApiKey, and Model - Replace hardcoded strings in BuildAppSettings method with constants - Improve code maintainability and consistency

…ouldly pattern - Replace Assert.Throws<T>() with Action.ShouldThrow<T>() for sync methods - Replace Assert.ThrowsAsync<T>() with Func<Task>.ShouldThrow<T>() for async methods - Remove async/await keywords from test methods using Shouldly pattern - Improve test readability with fluent API and method chaining - Ensure tests actually execute methods instead of just checking exceptions This addresses the bug where tests were not properly testing method execution and aligns with best practices for exception testing using Shouldly framework.

…ance-upstage - Merge latest changes from upstream repository - Includes HuggingFace connector implementation - UI improvements for ChatInput and ChatMessageItem components - Documentation updates and new connector guides - E2E tests for ChatInput IME functionality - Infrastructure updates for Azure deployment

- Add test to verify UpstageConnector inherits from LanguageModelConnector - Add using statement for OpenChat.PlaygroundApp.Abstractions namespace - Implement Theory test with InlineData for both inheritance directions: - LanguageModelConnector.IsAssignableFrom(UpstageConnector) should be true - UpstageConnector.IsAssignableFrom(LanguageModelConnector) should be false - Ensures proper polymorphism and type safety in connector architecture - Validates adherence to Liskov Substitution Principle

gremh97 · 2025-09-28T08:10:20Z

UpstageConnector가 LanguageModelConnector를 상속하는지에 대한 테스트가 ae18a9b에서 추가되었습니다

sikutisa

Exception 테스트 관련 수정 내용을 봤는데, Act & Assert로 묶은 이유가 있나요?
아래 처럼 분리할 수 있을 것 같은데요.

// Act
Action action = () => MethodToTest();

// Assert
action.ShouldThrow<SomethingException>()
      .Message.ShouldContain("something message");

sikutisa · 2025-09-29T15:28:29Z

테스트가 더 추가돼야 합니다.

UpstageConnector가 LanguageModelConnector를 상속하는지에 대한 테스트

테스트해야 하는 메소드는 총 3개입니다.

EnsureLanguageModelSettingsValid()

GetChatClientAsync()

LanguageModelConnector.CreateChatClientAsync()

이 내용 한 번 봐보시죠. #447 (comment)

이 부분도 더 보완돼야 합니다.

gremh97 · 2025-09-29T16:32:13Z

@sikutisa 기간이 지나서 없어졌는데 appsettings쪽 변수명 settings로 되어 있는건 그대로 유지하면 되는건가요?

sikutisa · 2025-09-29T16:33:57Z

@sikutisa 기간이 지나서 없어졌는데 appsettings쪽 변수명 settings로 되어 있는건 그대로 유지하면 되는건가요?

네. 일단 다른 커넥터랑 맞추시죠.

- Add test for GetChatClientAsync() with null settings scenario - Add test for LanguageModelConnector.CreateChatClientAsync() success case - Add test for LanguageModelConnector.CreateChatClientAsync() failure case - Complete test coverage for all public methods: * EnsureLanguageModelSettingsValid() - 5 test cases * GetChatClientAsync() - 5 test cases * CreateChatClientAsync() - 2 test cases * Inheritance relationship - 1 test case - Ensure proper exception handling and validation logic - Use Shouldly pattern for consistent and readable assertions

…ions - Separate combined '// Act & Assert' comments into individual sections - Improve test readability following AAA (Arrange-Act-Assert) pattern - Apply changes to 9 test methods across all test categories: * EnsureLanguageModelSettingsValid() tests (4 methods) * GetChatClientAsync() tests (4 methods) * CreateChatClientAsync() tests (1 method) - Maintain existing test logic while enhancing code structure - Better alignment with unit testing best practices

…ance-upstage

gremh97 · 2025-09-30T15:45:59Z

Exception 테스트 관련 수정 내용을 봤는데, Act & Assert로 묶은 이유가 있나요? 아래 처럼 분리할 수 있을 것 같은데요.
// Act
Action action = () => MethodToTest();

// Assert
action.ShouldThrow<SomethingException>()
      .Message.ShouldContain("something message");

넵 분리하였습니다

…nException - Change expected exception type from NullReferenceException to InvalidOperationException in GetChatClientAsync test - Fix test to match actual exception behavior when Upstage settings is null - All UpstageConnector tests now pass (23/23 successful)

gremh97 · 2025-09-30T16:03:12Z

테스트가 더 추가돼야 합니다.

UpstageConnector가 LanguageModelConnector를 상속하는지에 대한 테스트

테스트해야 하는 메소드는 총 3개입니다.

EnsureLanguageModelSettingsValid()

GetChatClientAsync()

LanguageModelConnector.CreateChatClientAsync()

이 내용 한 번 봐보시죠. #447 (comment)

이 부분도 더 보완돼야 합니다.

Given_Settings_Is_Null_When_GetChatClientAsync_Invoked_Then_It_Should_Throw
Given_Valid_Settings_When_CreateChatClientAsync_Invoked_Then_It_Should_Return_ChatClient
Given_Invalid_Settings_When_CreateChatClientAsync_Invoked_Then_It_Should_Throw
이렇게 세개의 테스트 케이스를 추가하였습니다.

UpstageConnector 테스트 케이스 정리 (총 13개)

1. 상속 관계 테스트 (1개)

Given_BaseType_Then_It_Should_Be_AssignableFrom_DerivedType
- 목적: UpstageConnector가 LanguageModelConnector를 올바르게 상속하는지 확인합니다.
- 테스트 메서드: Type.IsAssignableFrom()
- 예상 결과:
  LanguageModelConnector.IsAssignableFrom(UpstageConnector) // → true
  UpstageConnector.IsAssignableFrom(LanguageModelConnector) // → false

2. `EnsureLanguageModelSettingsValid()` 테스트 (5개)

Given_Settings_Is_Null_When_EnsureLanguageModelSettingsValid_Invoked_Then_It_Should_Throw
- 목적: Upstage 설정이 null일 때 예외 발생을 확인합니다.
- 예상 결과: InvalidOperationException 발생 (메시지에 "Upstage" 포함)
Given_Invalid_BaseUrl_When_EnsureLanguageModelSettingsValid_Invoked_Then_It_Should_Throw
- 목적: BaseUrl이 null, "", " "일 때 예외 발생을 확인합니다.
- 예상 결과: InvalidOperationException 발생 (메시지에 "Upstage:BaseUrl" 포함)
Given_Invalid_ApiKey_When_EnsureLanguageModelSettingsValid_Invoked_Then_It_Should_Throw
- 목적: ApiKey가 null, "", " "일 때 예외 발생을 확인합니다.
- 예상 결과: InvalidOperationException 발생 (메시지에 "Upstage:ApiKey" 포함)
Given_Invalid_Model_When_EnsureLanguageModelSettingsValid_Invoked_Then_It_Should_Throw
- 목적: Model이 null, "", " "일 때 예외 발생을 확인합니다.
- 예상 결과: InvalidOperationException 발생 (메시지에 "Upstage:Model" 포함)
Given_Valid_Settings_When_EnsureLanguageModelSettingsValid_Invoked_Then_It_Should_Return_True
- 목적: 모든 설정이 유효할 때 정상 동작을 확인합니다.
- 예상 결과: true 반환

3. `GetChatClientAsync()` 테스트 (5개)

Given_Valid_Settings_When_GetChatClient_Invoked_Then_It_Should_Return_ChatClient
- 목적: 유효한 설정으로 ChatClient 생성을 확인합니다.
- 예상 결과: ChatClient 객체가 null이 아님
Given_Settings_Is_Null_When_GetChatClientAsync_Invoked_Then_It_Should_Throw
- 목적: Upstage 설정이 null일 때 예외 발생을 확인합니다.
- 예상 결과: NullReferenceException 발생
Given_Missing_ApiKey_When_GetChatClient_Invoked_Then_It_Should_Throw
- 목적: ApiKey 누락 시 OpenAI SDK 예외 발생을 확인합니다.
- 예상 결과:
  - null → InvalidOperationException (메시지에 "Upstage:ApiKey" 포함)
  - "" → ArgumentException (메시지에 "key" 포함)
Given_Missing_BaseUrl_When_GetChatClient_Invoked_Then_It_Should_Throw
- 목적: BaseUrl 누락 시 URI 관련 예외 발생을 확인합니다.
- 예상 결과:
  - null → InvalidOperationException (메시지에 "Upstage:BaseUrl" 포함)
  - "" → UriFormatException (메시지에 "empty" 포함)
Given_Missing_Model_When_GetChatClient_Invoked_Then_It_Should_Throw
- 목적: Model 누락 시 OpenAI SDK 예외 발생을 확인합니다.
- 예상 결과:
  - null → ArgumentNullException (메시지에 "model" 포함)
  - "" → ArgumentException (메시지에 "model" 포함)

4. `CreateChatClientAsync()` 테스트 (2개)

Given_Valid_Settings_When_CreateChatClientAsync_Invoked_Then_It_Should_Return_ChatClient
- 테스트 메서드: LanguageModelConnector.CreateChatClientAsync()
- 목적: 팩토리 메서드로 정상적인 ChatClient 생성을 확인합니다.
- 예상 결과: ChatClient 객체가 null이 아님
Given_Invalid_Settings_When_CreateChatClientAsync_Invoked_Then_It_Should_Throw
- 테스트 메서드: LanguageModelConnector.CreateChatClientAsync()
- 목적: 잘못된 설정(ApiKey = null)일 때 예외 발생을 확인합니다.
- 예상 결과: InvalidOperationException 발생 (메시지에 "Upstage:ApiKey" 포함)

sikutisa

같은 메소드에 대한 테스트끼리 모아 놓는 게 좋을 것 같습니다.
올려주신 댓글 순서대로 맞춰도 될 것 같은데요?

test/OpenChat.PlaygroundApp.Tests/Connectors/UpstageConnectorTests.cs

…lity - Group GetChatClient related tests together in logical sections - Maintain all existing test functionality and coverage - Improve test file structure and maintainability - No changes to test logic or assertions

- Convert single Fact test to Theory with multiple test cases - Add comprehensive validation for all required settings: ApiKey, BaseUrl, Model - Test null value handling for each parameter individually - Ensure proper InvalidOperationException thrown with specific error messages - Improve test coverage from 1 to 3 validation scenarios Changes: - Replace Given_Invalid_Settings_When_CreateChatClientAsync_Invoked_Then_It_Should_Throw Fact with Theory - Add InlineData for apiKey, baseUrl, and model null validation - Use switch expression for dynamic AppSettings creation - Verify exception type and message content for each case

…attern - Update exception message validation to match OpenAI connector format - Change "Upstage" to "Missing configuration: Upstage." for EnsureLanguageModelSettingsValid - Add specific message validation "Missing configuration: Upstage:ApiKey." for GetChatClientAsync null settings - Improve consistency across connector error messages - Enhance test robustness by validating both exception type and specific message content Benefits: - Consistent error message format across all connectors - More descriptive error messages for better debugging - Follows established OpenAI connector pattern - Better test coverage with message content validation

…ance-upstage

test/OpenChat.PlaygroundApp.Tests/Connectors/UpstageConnectorTests.cs

…entAsync - Expand test coverage from 3 to 12 test cases for CreateChatClientAsync validation - Add empty string ("") validation for all parameters (apiKey, baseUrl, model) - Add whitespace validation (" ") for all parameters - Add mixed whitespace validation ("\t\n\r") for all parameters - Ensure robust handling of null, empty, and whitespace-only values - Maintain consistent InvalidOperationException with specific error messages Enhanced test scenarios: - apiKey: null, "", " ", "\t\n\r" → InvalidOperationException: "Upstage:ApiKey" - baseUrl: null, "", " ", "\t\n\r" → InvalidOperationException: "Upstage:BaseUrl" - model: null, "", " ", "\t\n\r" → InvalidOperationException: "Upstage:Model" Benefits: - Comprehensive validation coverage for all invalid input types - Better protection against configuration errors in production - Consistent with other connector validation patterns

- Change nullValue to invalidValue in CreateChatClientAsync test method - Better reflects comprehensive test coverage including null, empty strings, and whitespace variations - Improves code clarity and test intent documentation

sikutisa

이제 Bicep 작업 시작하시면 될 것 같습니다!

gremh97 added 3 commits September 22, 2025 00:27

Merge branch 'main' into feature/235-connector-implementation-inherit…

114e6e1

…ance-upstage

Add UpstageConnector unit tests

095a6d1

- Added test/OpenChat.PlaygroundApp.Tests/Connectors/UpstageConnectorTests.cs - Covers configuration validation, error handling, and ChatClient creation for Upstage connector - Ensures reliability and correct behavior for Upstage integration

sikutisa self-requested a review September 23, 2025 13:30

sikutisa assigned gremh97 Sep 23, 2025

sikutisa added ossca p1 lead phase-1 task vendor labels Sep 23, 2025

sikutisa requested changes Sep 25, 2025

View reviewed changes

gremh97 added 4 commits September 28, 2025 15:49

aliencube#235 Replace magic strings with constants in UpstageConnecto…

90aec64

…rTests - Add private const fields for BaseUrl, ApiKey, and Model - Replace hardcoded strings in BuildAppSettings method with constants - Improve code maintainability and consistency

sikutisa requested changes Sep 29, 2025

View reviewed changes

gremh97 added 3 commits October 1, 2025 00:36

Merge branch 'main' into feature/235-connector-implementation-inherit…

f3c67a9

…ance-upstage

sikutisa requested changes Oct 1, 2025

View reviewed changes

gremh97 added 2 commits October 4, 2025 01:35

gremh97 added 2 commits October 4, 2025 17:34

Merge branch 'main' into feature/235-connector-implementation-inherit…

982f381

…ance-upstage

sikutisa requested changes Oct 4, 2025

View reviewed changes

test/OpenChat.PlaygroundApp.Tests/Connectors/UpstageConnectorTests.cs Show resolved Hide resolved

gremh97 added 2 commits October 5, 2025 02:02

sikutisa reviewed Oct 5, 2025

View reviewed changes

Feature/235 connector implementation inheritance upstage #463

Are you sure you want to change the base?

Feature/235 connector implementation inheritance upstage #463

Conversation

gremh97 commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Does this introduce a breaking change?

Pull Request Type

README updated?

How to Test

What to Check

Other Information

Uh oh!

sikutisa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gremh97 commented Sep 28, 2025

Uh oh!

sikutisa left a comment

Choose a reason for hiding this comment

Uh oh!

sikutisa commented Sep 29, 2025

Uh oh!

gremh97 commented Sep 29, 2025

Uh oh!

sikutisa commented Sep 29, 2025

Uh oh!

gremh97 commented Sep 30, 2025

Uh oh!

gremh97 commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

UpstageConnector 테스트 케이스 정리 (총 13개)

1. 상속 관계 테스트 (1개)

2. EnsureLanguageModelSettingsValid() 테스트 (5개)

3. GetChatClientAsync() 테스트 (5개)

4. CreateChatClientAsync() 테스트 (2개)

Uh oh!

sikutisa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sikutisa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gremh97 commented Sep 23, 2025 •

edited

Loading

gremh97 commented Sep 30, 2025 •

edited

Loading

2. `EnsureLanguageModelSettingsValid()` 테스트 (5개)

3. `GetChatClientAsync()` 테스트 (5개)

4. `CreateChatClientAsync()` 테스트 (2개)