Resolve #487: Enhance tests and fix linting issues

[ealvar3z]

Describe the change

• Add randomized fields to Engine structs in tests

Describe your solution

• Implement cryptographically secure random utility functions

Tests

• Make these structs non-empty, by randomizing them, while keeping randomization logic in the internal/test directory (internal/test/random.go). So in the end we'll get something like:

server.RegisterHandler("/v1/engines/text-davinci-003", func(w http.ResponseWriter, r *http.Request) {
	resBytes, _ := json.Marshal(Engine{ID: test.randomString(), Object: test.randomString()})
	fmt.Fprintln(w, string(resBytes))
})

• Address golangci-lint warnings

Additional context

• None

Issue: #487

[codecov]

Codecov Report

Merging #491 (5341647) into master (3589837) will increase coverage by 0.28%.
Report is 7 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #491      +/-   ##
==========================================
+ Coverage   97.30%   97.58%   +0.28%     
==========================================
  Files          18       19       +1     
  Lines         780      871      +91     
==========================================
+ Hits          759      850      +91     
  Misses         15       15              
  Partials        6        6              

Files	Coverage Δ
embeddings.go	100.00% <100.00%> (ø)

... and 14 files with indirect coverage changes

[sashabaranov]

Thank you for the PR!

[sashabaranov]

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)

[ealvar3z]

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.

[sashabaranov]

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

[ealvar3z]

Done. See PR #509

[sashabaranov]

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

[ealvar3z]

Done

[sashabaranov]

@ealvar3z it's not removed :D

[sashabaranov]

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

[sashabaranov]

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

[ealvar3z]

Will do. Thank you for your patience.