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

Use Assert package in the test files

Open rexposadas opened this issue 1 year ago • 6 comments

Rationale

I suggest using the assert package in the tests. See: https://github.com/stretchr/testify

It'll make the code cleaner. For example in api_test.go we have:

	if err == nil {
		t.Fatal("ListEngines did not fail")
	}
```	

That can be changed to: 

```go	
assert.NoErrorf(t, err, "ListEngines did not fail")

Implementation

I've implemented this feature for production level code and it makes the test code look a lot cleaner.

If you agree with the suggestion, I am willing to make the PR for it.

rexposadas avatar Mar 20 '23 15:03 rexposadas

@rexposadas Thank you for the suggestion! Our library aims to have no dependencies for as long as possible. While the testify library has some helpful features, they are not enough to make it a dependency. Having said that — we can add the single-line error checking in tests to the library. Your contribution would be greatly appreciated, along with any code coverage improvements 🙌🏻

sashabaranov avatar Mar 20 '23 15:03 sashabaranov

I see. No problem.

we can add the single-line error checking in tests to the library

Which library are you referring to? Would it be this one? https://github.com/sashabaranov/go-fastapi

rexposadas avatar Mar 20 '23 15:03 rexposadas

@rexposadas I mean to this library, just as a function e.g.

func checkNoError(t *testing.T, err error, message string) {
	if err != nil {
		t.Errorf(message)
	}
}

sashabaranov avatar Mar 20 '23 15:03 sashabaranov

@sashabaranov But, to keep the code consistent would mean making other functions such as:

checkHasError (..), checkErrorIs(..)

You're OK with that?

rexposadas avatar Mar 20 '23 15:03 rexposadas

@rexposadas sounds good! It might also make sense to put those into internal/test package, but I trust your judgment here.

sashabaranov avatar Mar 20 '23 16:03 sashabaranov

Great. I'll work on the PR.

rexposadas avatar Mar 20 '23 17:03 rexposadas

PR for this has been merged.

rexposadas avatar Mar 26 '23 15:03 rexposadas