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

Refactor Claude Endpoint, introduce Must0 utility, and add tests #69

Merged
merged 14 commits into from
Mar 25, 2025

Conversation

nvtrinh2001
Copy link
Collaborator

@nvtrinh2001 nvtrinh2001 commented Mar 18, 2025

Summary

This PR does:

  • refactor the Claude Endpoint implementation to use dependency injection (DI) instead of directly coupling to the anthropic.Client struct
  • add tests for Claude Endpoint
  • add util function Must0 to guarantee not producing errors
  • add tests for Must0

@nvtrinh2001 nvtrinh2001 mentioned this pull request Mar 19, 2025
Copy link
Collaborator

@seungduk-yanolja seungduk-yanolja left a comment

Choose a reason for hiding this comment

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

The convention is to have a test file for each go file.

@nvtrinh2001
Copy link
Collaborator Author

@seungduk-yanolja thank you for the feedback. For the Ping and GenerateChatCompletion methods, since the client field in Endpoint is not an interface, but a pointer to the anthropic.Client struct, I cannot inject mock to create unit tests for them.

Here is the current code:

type Endpoint struct {
	client *anthropic.Client
}
func (ep *Endpoint) Ping(ctx context.Context) (time.Duration, error) {
	start := time.Now()
	_, err := ep.client.Messages.New(ctx, anthropic.MessageNewParams{
		Model:     anthropic.F(anthropic.ModelClaude_3_Haiku_20240307),
		MaxTokens: anthropic.Int(1),
		Messages: anthropic.F([]anthropic.MessageParam{
			anthropic.NewUserMessage(anthropic.NewTextBlock("Ping")),
		}),
	})
	if err != nil {
		return 0, err
	}
	return time.Since(start), nil
}

What is your recommendation to solve it? Should I change the current code to make it easier to test, or ignore it and write integration test with real API call instead?

@nvtrinh2001
Copy link
Collaborator Author

nvtrinh2001 commented Mar 20, 2025

@seungduk-yanolja I updated the claude.go file to be more testable. I also wrote a test file for it and removed unnecessary tests for private methods.

@nvtrinh2001 nvtrinh2001 changed the title test: add tests for claude provider update: refactor claude implementation to be more test-friendly Mar 20, 2025
@nvtrinh2001 nvtrinh2001 changed the title update: refactor claude implementation to be more test-friendly refactor Claude endpoint to use dependency injection for improved testability Mar 20, 2025
Copy link
Collaborator

@seungduk-yanolja seungduk-yanolja left a comment

Choose a reason for hiding this comment

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

Please capitalize the first letter of the PR title

@nvtrinh2001 nvtrinh2001 changed the title refactor Claude endpoint to use dependency injection for improved testability Refactor Claude endpoint to use dependency injection for improved testability Mar 21, 2025
@nvtrinh2001 nvtrinh2001 changed the title Refactor Claude endpoint to use dependency injection for improved testability Refactor Claude Endpoint, introduce Must0 utility, and add tests Mar 21, 2025
@nvtrinh2001 nvtrinh2001 merged commit e43290e into yanolja:main Mar 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants