-
Notifications
You must be signed in to change notification settings - Fork 5
Add test coverage for OpenAI completion provider #72
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
base: main
Are you sure you want to change the base?
Conversation
|
Let me work on the failing checks and raise a new rev |
|
@Furisto Can you review this ? |
| } | ||
| } | ||
|
|
||
| func TestNewOpenAICompletionProviderWithService(t *testing.T) { |
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.
Can we remove this test? It does not provide much value.
| } | ||
| } | ||
|
|
||
| func TestOpenAICompletionProvider_InvokeModel_ModelProfileValidation(t *testing.T) { |
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.
Would prefer to write a separate test for the Validate function of the model profile and not intermingle it with the InvokeModel test.
| // Edge Cases Tests | ||
| // ============================================================================= | ||
|
|
||
| func TestOpenAICompletionProvider_TransformMessages_EdgeCases(t *testing.T) { |
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.
This should be part of the TransformMessages test instead of a separate test for edge cases.
| } | ||
| } | ||
|
|
||
| func TestOpenAICompletionProvider_TransformTools_ManyTools(t *testing.T) { |
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.
Let's drop this test. We already have tests that cover transformation of tools and testing that n+1 works when we already test n does not contribute much value.
| var wg sync.WaitGroup | ||
| errorChan := make(chan error, 100) | ||
|
|
||
| for i := 0; i < 100; i++ { |
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.
Validation of inputs is stateless. What scenario would this cover?
| return m.stream | ||
| } | ||
|
|
||
| func (m *mockChatCompletionService) getCallCount() int { |
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.
When do we call this method?
| } | ||
| } | ||
|
|
||
| func TestOpenAICompletionProvider_ConcurrentTransformMessages(t *testing.T) { |
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.
Why do we need to test concurrent transformation of messages?
| } | ||
| } | ||
|
|
||
| // ============================================================================= |
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.
Nit: We do not need the comment. The test name is enough.
Summary
Fixes #68
OpenAIChatCompletionServiceinterface to enable mocking in testsNewOpenAICompletionProviderWithService()constructor for test injectionTest Coverage
Test Plan
go test ./backend/model/...)go build ./backend/... ./api/go/... ./frontend/cli/... ./shared/...)