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.

Sign up for GitHub

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

Jump to bottom

Resolve #487: Enhance tests and fix linting issues #491

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Resolve #487: Enhance tests and fix linting issues #491

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
57a4fb4
Resolve #487: Enhance tests and fix linting issues
ealvar3z Sep 12, 2023
bd1bb20
Rewrite Cryptographically Secure RandomString Function
ealvar3z Sep 14, 2023
d8c78aa
Create RandomInt Function
ealvar3z Sep 14, 2023
dbb476e
Resolve code review comments in PR #491
ealvar3z Sep 26, 2023
5341647
Remove test.Seed(42) per review feedback
ealvar3z Oct 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions engines_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,27 +8,51 @@ import (
"testing"

. "github.com/sashabaranov/go-openai"
"github.com/sashabaranov/go-openai/internal/test"
"github.com/sashabaranov/go-openai/internal/test/checks"
)

// Helper function to test the ListEngines endpoint.
func RandomEngine() Engine {
return Engine{
ID: test.RandomString(),
Object: test.RandomString(),
Owner: test.RandomString(),
Ready: test.RandomBool(),
}
}

// TestGetEngine Tests the retrieve engine endpoint of the API using the mocked server.
func TestGetEngine(t *testing.T) {
test.Seed(42) // Seed the RNG
client, server, teardown := setupOpenAITestServer()
defer teardown()

expectedEngine := RandomEngine() // move outside of handler per code review comment
Copy link
Owner

Choose a reason for hiding this comment

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

(2) Could we please remove these comments? They are relevant to review but not for the codebase going forward

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do. Thank you for your patience.

server.RegisterHandler("/v1/engines/text-davinci-003", func(w http.ResponseWriter, r *http.Request) {
resBytes, _ := json.Marshal(Engine{})
resBytes, _ := json.Marshal(expectedEngine)
fmt.Fprintln(w, string(resBytes))
})
_, err := client.GetEngine(context.Background(), "text-davinci-003")
actualEngine, err := client.GetEngine(context.Background(), "text-davinci-003")
checks.NoError(t, err, "GetEngine error")

// Compare the two using only one field per code review comment
Copy link
Owner

Choose a reason for hiding this comment

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

Could we please remove these comments? They are relevant to review, but not for the codebase going forward

if actualEngine.ID != expectedEngine.ID {
t.Errorf("Engine ID mismatch: got %s, expected %s", actualEngine.ID, expectedEngine.ID)
}
}

// TestListEngines Tests the list engines endpoint of the API using the mocked server.
func TestListEngines(t *testing.T) {
test.Seed(42) // Seed the RNG
Copy link
Owner

Choose a reason for hiding this comment

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

Thank you for the updates, that looks perfect! Could we keep seeding logic out for now, I think it would be useful in cases we'll actually need to reproduce something (e.g. fuzzing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review, @sashabaranov.

I've can go ahead and remove the RNG seeding from TestListEngines() as you suggested. I agree that there could be specific scenarios like fuzz testing where non-deterministic behavior is desirable.

// Possible compromise scenario though
We could introduce a flag or environment variable to decide whether or not to seed the RNG, allowing us to keep both deterministic and non-deterministic behavior depending on the use-case. Would that be an acceptable solution for future requirements?

Here's what I am thinking:

func MaybeSeedRNG() {
	seed := os.Getenv("TEST_RNG_SEED")

	if seedValue, err := strconv.ParseInt(seed, 10, 64); err == nil {
		rand.Seed(seedValue)
		return
	}

	// However, if the environment variable is set to "random", 
	// we can use the current time as the seed
	if seedEnv == "random" {
		rand.Seed(time.Now().UnixNano())
	}
}

With this setup, we can control the RNG seeding behavior through the TEST_RNG_SEED environment variable:

So to make the tests deterministic with a specific seed, we can set the environment variable to that seed (e.g., TEST_RNG_SEED=42). To make tests non-deterministic, we can set the environment variable to "random" (TEST_RNG_SEED=random).

If we don't set the environment variable, the RNG will not be seeded, maintaining its default non-deterministic behavior.

Then, we could call it whenever we want determinism, e.g:

func TestListEngines(t *testing.T) {
	test.MaybeSeedRNG()  // Call it here
	client, server, teardown := setupOpenAITestServer()
	defer teardown()
	// ... rest of the function remains the same ...
}

Please advise.

Copy link
Owner

Choose a reason for hiding this comment

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

@ealvar3z I like the MaybeSeedRNG approach! Let's factor it out to the separate PR, trying to keep the PR size small-ish

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. See PR #509

Copy link
Owner

Choose a reason for hiding this comment

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

Alright, let's remove test.Seed(42) from this PR and merge!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Owner

Choose a reason for hiding this comment

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

@ealvar3z it's not removed :D

client, server, teardown := setupOpenAITestServer()
defer teardown()
server.RegisterHandler("/v1/engines", func(w http.ResponseWriter, r *http.Request) {
resBytes, _ := json.Marshal(EnginesList{})
engines := make([]Engine, test.RandomInt(5))
for i := range engines {
engines[i] = RandomEngine()
}
resBytes, _ := json.Marshal(EnginesList{Engines: engines})
fmt.Fprintln(w, string(resBytes))
})
_, err := client.ListEngines(context.Background())
Expand Down
48 changes: 48 additions & 0 deletions internal/test/random.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package test

import (
"math/rand"
"strings"
"time"
)

const letters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"
const strLen = 10

// #nosec G404
var r = rand.New(rand.NewSource(time.Now().UnixNano()))

// Seeding func.
// #nosec G404
func Seed(s int64) {
r = rand.New(rand.NewSource(s))
}

// See StackOverflow answer:
// https://stackoverflow.com/questions/22892120/how-to-generate-a-random-string-of-a-fixed-length-in-go
// RandomString generates a random string of length
// strLen.
func RandomString() string {
sb := strings.Builder{}
sb.Grow(strLen)

for i := 0; i < strLen; i++ {
randomIndex := r.Intn(len(letters))
sb.WriteByte(letters[randomIndex])
}

return sb.String()
}

// RandomInt generates a random integer between 0 (inclusive) and 'max'
// (exclusive).
func RandomInt(max int) int {
return r.Intn(max)
}

// RandomBool generates a random boolean value.
// #nosec G404
func RandomBool() bool {
n := 2 // #gomnd (golangci-lint magic number suppression)
return r.Intn(n) == 1
}
Loading