-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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: Minor improvements to BackendConfigLoader #2353
Changes from 3 commits
e94e093
7953edf
dd89826
d852762
d8961e5
827e59c
4c616f9
afe0a61
687be53
8403d14
869687a
12f4487
692ee3f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,8 @@ | ||
package config_test | ||
package config | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. according to golang docs, since this file ends in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this is more a bit of a code/style in writing tests. I always prefer (especially in go) to go with black box testing rather then white box testing - in the long run black box tests are less brittle and allow me to test the intention rather then corner cases, but I do agree that here things are quite shady and can be improved |
||
import ( | ||
"os" | ||
|
||
. "github.com/go-skynet/LocalAI/core/config" | ||
|
||
. "github.com/onsi/ginkgo/v2" | ||
. "github.com/onsi/gomega" | ||
) | ||
|
@@ -17,8 +15,8 @@ var _ = Describe("Test cases for config related functions", func() { | |
|
||
Context("Test Read configuration functions", func() { | ||
configFile = os.Getenv("CONFIG_FILE") | ||
It("Test ReadConfigFile", func() { | ||
config, err := ReadBackendConfigFile(configFile) | ||
It("Test readConfigFile", func() { | ||
config, err := readMultipleBackendConfigsFromFile(configFile) | ||
Expect(err).To(BeNil()) | ||
Expect(config).ToNot(BeNil()) | ||
// two configs in config.yaml | ||
|
@@ -27,26 +25,31 @@ var _ = Describe("Test cases for config related functions", func() { | |
}) | ||
|
||
It("Test LoadConfigs", func() { | ||
cm := NewBackendConfigLoader() | ||
bcl := NewBackendConfigLoader() | ||
opts := NewApplicationConfig() | ||
err := cm.LoadBackendConfigsFromPath(opts.ModelPath) | ||
err := bcl.LoadBackendConfigsFromPath(opts.ModelPath) | ||
Expect(err).To(BeNil()) | ||
Expect(cm.ListBackendConfigs()).ToNot(BeNil()) | ||
configs := bcl.GetAllBackendConfigs() | ||
loadedModelNames := []string{} | ||
for _, v := range configs { | ||
loadedModelNames = append(loadedModelNames, v.Name) | ||
} | ||
Expect(configs).ToNot(BeNil()) | ||
|
||
// config should includes gpt4all models's api.config | ||
Expect(cm.ListBackendConfigs()).To(ContainElements("gpt4all")) | ||
Expect(loadedModelNames).To(ContainElements("gpt4all")) | ||
|
||
// config should includes gpt2 models's api.config | ||
Expect(cm.ListBackendConfigs()).To(ContainElements("gpt4all-2")) | ||
Expect(loadedModelNames).To(ContainElements("gpt4all-2")) | ||
|
||
// config should includes text-embedding-ada-002 models's api.config | ||
Expect(cm.ListBackendConfigs()).To(ContainElements("text-embedding-ada-002")) | ||
Expect(loadedModelNames).To(ContainElements("text-embedding-ada-002")) | ||
|
||
// config should includes rwkv_test models's api.config | ||
Expect(cm.ListBackendConfigs()).To(ContainElements("rwkv_test")) | ||
Expect(loadedModelNames).To(ContainElements("rwkv_test")) | ||
|
||
// config should includes whisper-1 models's api.config | ||
Expect(cm.ListBackendConfigs()).To(ContainElements("whisper-1")) | ||
Expect(loadedModelNames).To(ContainElements("whisper-1")) | ||
}) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlike most of the other moves, this function was removed. All places that used it have been converted to
GetAllBackendConfigs()
- the one place in live code essentially did that anyway, and the test has been modified.