go-openai icon indicating copy to clipboard operation
go-openai copied to clipboard

Add a Mocked Server for Testing

Open RobotSail opened this issue 3 years ago • 10 comments

Based on a request from #27, we should introduce a mocked server which tests that values are correctly sent by go-gpt3 and received by the server, as well as the ability for the client to receive data from the server.

This can help us to quickly iterate on tests without spending any OpenAI credits.

We need the following to complete this task

  • [x] Create a mocked OpenAI API server
  • [ ] Write endpoint handlers and tests for the mocked server:
    • [x] Completions
    • [x] Edits
    • [ ] Moderation
    • [ ] Search
    • [ ] Embeddings
    • [ ] Answers
    • [ ] Files
    • [ ] Classifications

RobotSail avatar Aug 02 '22 23:08 RobotSail

@RobotSail Do you need any help on this?

vimitdhawan avatar Jan 26 '23 03:01 vimitdhawan

Hi @vimitdhawan, we merged #31 last year, and I haven't touched the others once since. If you wanted to contribute some tests for these other points, that would be amazing.

RobotSail avatar Jan 26 '23 13:01 RobotSail

The testing of the file is not done yet, can I contribute a bit to this?

Rascal0814 avatar Feb 08 '23 05:02 Rascal0814

The testing of the file is not done yet, can I contribute a bit to this?

And I think it is unreasonable to put all the tests in the api_test file, and the functions of the same nature should be put in one file

Rascal0814 avatar Feb 08 '23 05:02 Rascal0814

@RobotSail Hi. Client should be an interface to a client which implements the methods. That would solve all your testing problems. Then you could just setup a mock on your interface. Here's a simple example:

type Client interface {
    ListEngines(ctx context.Context) (engines EnginesList, err error)
}

type client struct {
    config ClientConfig
}

func (c *client) ListEngines(ctx context.Context) (engines EnginesList, err error) {
...
}

type MockClient struct {
    mock.Mock
}

func (c *MockClient) ListEngines(ctx context.Context) (engines EnginesList, err error) {
    args := c.Called(ctx)
    return args.Get(0).(EnginesList), args.Error(1)
}

This is untested but should be the general format. I'd be happy to try to take it on, it would just be a pretty large refactor but would make it easier for users (like me) to test the Client.

drkennetz avatar Mar 01 '23 02:03 drkennetz

@drkennetz, the approach you are talking about would generally be correct if there were some business logic in Client. However, Client is the whole business logic we have here, you don't get it tested if you mock it.

sashabaranov avatar Mar 01 '23 10:03 sashabaranov

After some further thought, do we really even need the rest of these tests? The whole point of this package is providing a client to call OpenAI's API, so would it even make sense to have a goal of some % of code coverage? All that is being done here is the API gets called and then returns the results.

RobotSail avatar Mar 01 '23 14:03 RobotSail

The goal of mock's is never to test an API call, it is to ensure that changes in internal software don't break your code. They provide fast iteration if you ever decide to change your code, and they provide consistent results.

I don't understand what you mean by saying that "you don't get it tested if you mock it". Yeah, you don't test that you actually make an API call to openai, but you test the method takes in and puts out data correctly.

There is a lot of software that is "just an API call" and contains nothing except a Get request to a database, and that software should absolutely be wrapped in tests. It is also core to the business.

Arguably, it doesn't make sense to make "actual" api requests to a third party as you have no control over them changing the API, there is no way you can guarantee consistent results, and it will be abundantly apparent to you and everybody else if the API stops working like it used to. All you're really checking by getting an actual response is that you can connect to the API.

drkennetz avatar Mar 02 '23 22:03 drkennetz

@drkennetz got it, thank you! From your example above, it appears that you've proposed to mock a client like this https://github.com/sashabaranov/go-gpt3/blob/39ca4e94882215d59857cd8791c12082edec2c97/engines.go#L24 which would defeat the purpose of testing the business logic in the client.

From your last message, I understand that you propose to mock HTTPClient here https://github.com/sashabaranov/go-gpt3/blob/master/config.go#L16 and making it mock server responses, basically substituting call to a mock server to just returning the result.

Would love to merge your PR implementing that! 🙌🏻

sashabaranov avatar Mar 03 '23 06:03 sashabaranov

Sorry for the lack of clarity in my original post!

I'd be happy to do that. Maybe I'll start with a small one as an example, and if we're happy with it I can implement across the board 👍

drkennetz avatar Mar 03 '23 07:03 drkennetz

@sashabaranov Mock testing of all OpenAI APIs was implemented with a pull request https://github.com/sashabaranov/go-openai/pull/356 . Did you meet the requirements of this Issue? Can you close this issue?

vvatanabe avatar Jun 12 '23 21:06 vvatanabe

Fixed #356

vvatanabe avatar Jul 01 '23 08:07 vvatanabe