test: create initial Integration tests #4895

ukclivecox · 2023-06-07T18:06:00Z

Initial integration tests PoC

Updates CLI to simplfy usage to allow simple Load, Unload, Status, Infer to be used easily from integration tests
Add k8s integration for Load, Unload, Status, Infer
Adds a generic SeldonBackedAPI getter for tests to get either docker or k8s integration to test locally or via k8s. Defaults to local docker integration if it finds it first.
Adds initial example tests for loading, waiting for ready, infer and unload of Model

make test
go test -v ./pkg/... -coverprofile cover.out
=== RUN   TestSingleModelLoadInferUnload
=== RUN   TestSingleModelLoadInferUnload/rest
=== RUN   TestSingleModelLoadInferUnload/grpc
=== RUN   TestSingleModelLoadInferUnload/unknown
--- PASS: TestSingleModelLoadInferUnload (0.00s)
    --- PASS: TestSingleModelLoadInferUnload/rest (0.00s)
    --- PASS: TestSingleModelLoadInferUnload/grpc (0.00s)
    --- PASS: TestSingleModelLoadInferUnload/unknown (0.00s)
PASS
coverage: 8.6% of statements
ok  	github.com/seldonio/seldon-core/tests/integration/v2/pkg/resources	0.013s	coverage: 8.6% of statements
=== RUN   TestSingleModelLoadInferUnload
=== RUN   TestSingleModelLoadInferUnload/sklearn_-_iris
    model_test.go:46: Waiting for model testdata/sklearn-iris.yaml:false
    model_test.go:46: Waiting for model testdata/sklearn-iris.yaml:true
=== RUN   TestSingleModelLoadInferUnload/tensorflow_-_tfsimple
    model_test.go:46: Waiting for model testdata/tensorflow-tfsimple.yaml:false
    model_test.go:46: Waiting for model testdata/tensorflow-tfsimple.yaml:true
=== RUN   TestSingleModelLoadInferUnload/xgboost_-_income
    model_test.go:46: Waiting for model testdata/xgboost-income.yaml:false
    model_test.go:46: Waiting for model testdata/xgboost-income.yaml:true
--- PASS: TestSingleModelLoadInferUnload (3.95s)
    --- PASS: TestSingleModelLoadInferUnload/sklearn_-_iris (1.02s)
    --- PASS: TestSingleModelLoadInferUnload/tensorflow_-_tfsimple (1.03s)
    --- PASS: TestSingleModelLoadInferUnload/xgboost_-_income (1.04s)
PASS
coverage: [no statements]
ok  	github.com/seldonio/seldon-core/tests/integration/v2/pkg/tests	3.962s	coverage: [no statements]

Notes:

Initial investigation to use go testcontainers with compose. However, gave up as that tries to use googleapis/gnostic via 3rd party dependency and we use more recent google/gnostic. As both define protos fails by default due to proto clash. This can be overridden but for not left the ability to create Seldon via Compose as part of tests as not done and assume its already installed by some previous method.

review-notebook-app · 2023-06-09T14:44:52Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

… default

agrski

Various clarifications and changes suggestions requested, as well as a few suggestions for now and the future.

As general comments from offline discussion:

Relying on an existing setup from the environment is fine for a first pass to show the functionality, but highly brittle for actually running integration tests.
We should add in mechanisms to isolate test cases so they aren't accidental interactions between them, e.g. leaving a model loaded.
We should be clear about whether we expect the test to run in k8s or local Docker/Compose rather than trying to infer this too.
Using hard-coded endpoints is also awkward for running different test cases concurrently.
It'd be nice in the future to separate the definition of test scenarios from the runner code, if possible. This would make it easier for non-Go devs to write and understand test cases.

operator/cmd/seldon/cli/experiment_start.go

agrski · 2023-06-09T10:19:17Z

operator/cmd/seldon/cli/model_status.go

-	flags.BoolP(flagShowRequest, "r", false, "show request")
-	flags.BoolP(flagShowResponse, "o", true, "show response")
+	flags.Int64P(flagTimeout, "t", flagTimeoutDefault, "timeout seconds")
+	flags.BoolP(flagVerbose, "v", false, "verbose output")
 	flags.String(flagSchedulerHost, env.GetString(envScheduler, defaultSchedulerHost), helpSchedulerHost)
 	flags.String(flagAuthority, "", helpAuthority)
 	flags.StringP(flagWaitCondition, "w", "", "model wait condition")


💭 Just noticed this - experiment_status has wait as a Boolean, whereas here the user is expected to know what condition to apply. For pipelines I can understand that there are two underlying conditions (pipeline infra + models), but here it's slightly awkward, no?

Not directly related to this PR, but thought I'd raise anyway

Yes could change or consolidate in a future PR

Leaving unresolved to track

operator/cmd/seldon/cli/pipeline_inspect.go

operator/pkg/cli/infer.go

operator/pkg/cli/generic.go

agrski · 2023-06-09T16:53:40Z

tests/integration/pkg/tests/model_test.go

+)
+
+func TestSingleModelLoadInferUnload(t *testing.T) {
+	g := NewGomegaWithT(t)


💭 Not a change for this PR, but I'd have a preference for using testify for assertions instead of Gomega as it's lighter, simpler, and less verbose. One to discuss offline perhaps

tests/integration/pkg/tests/model_test.go

agrski · 2023-06-09T16:56:48Z

tests/integration/pkg/tests/model_test.go

+	sapi, err := resources.NewSeldonBackendAPI()
+	g.Expect(err).To(BeNil())
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {


💭 This could be more nicely expressed as multiple subtests with t.Run() to capture the load, infer, and unload parts. That way, if something fails then it's immediately clear which bit failed

Maybe future PR

Sure - it's an easy and small enough change to leave. Leaving comment unresolved as a reminder

tests/integration/pkg/tests/testdata/tensorflow-tfsimple-request.json

tests/integration/pkg/tests/testdata/sklearn-iris.yaml

tests/integration/pkg/resources/seldon_api.go

operator/pkg/cli/scheduler.go

ukclivecox · 2023-06-13T17:33:31Z

Review fixes and also added build target for integration so just make test does internal tests only (no seldon required)

tests/integration/.golangci.yml

agrski

One minor change left, but approving as this does not affect functionality if not done -- just confusing/surprising to see

ukclivecox added 2 commits June 7, 2023 18:49

initial integration tests PoC

a12a89b

Merge branch 'v2' into integration-tests

296a8c8

ukclivecox added the v2 label Jun 7, 2023

ukclivecox requested a review from agrski June 7, 2023 18:06

ukclivecox changed the title ~~Integration tests~~ test: create initial Integration tests Jun 7, 2023

ukclivecox added 3 commits June 7, 2023 19:13

lint

10ca2dc

Merge branch 'v2' into integration-tests

aefc092

Add infer response initial testing

dfed824

ukclivecox added 2 commits June 9, 2023 15:51

revert notebook accidental commit

4063320

revert status to have verbose is true so you can see return status by…

3e700fe

… default

agrski reviewed Jun 9, 2023

View reviewed changes

ukclivecox added 2 commits June 10, 2023 15:30

review fixes

a797413

review fixes

3948e7b

agrski reviewed Jun 12, 2023

View reviewed changes

tests/integration/pkg/resources/seldon_api.go Outdated Show resolved Hide resolved

agrski reviewed Jun 12, 2023

View reviewed changes

operator/pkg/cli/scheduler.go Show resolved Hide resolved

agrski reviewed Jun 12, 2023

View reviewed changes

operator/pkg/cli/scheduler.go Show resolved Hide resolved

review fixes and build flag for integration tests

2f06bdf

agrski reviewed Jun 14, 2023

View reviewed changes

tests/integration/.golangci.yml Outdated Show resolved Hide resolved

agrski approved these changes Jun 14, 2023

View reviewed changes

fix typo in golintci

1e55114

ukclivecox merged commit 623b792 into SeldonIO:v2 Jun 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: create initial Integration tests #4895

test: create initial Integration tests #4895

ukclivecox commented Jun 7, 2023

review-notebook-app bot commented Jun 9, 2023

agrski left a comment

agrski Jun 9, 2023

ukclivecox Jun 10, 2023 •

edited

Loading

agrski Jun 12, 2023

agrski Jun 9, 2023

agrski Jun 9, 2023

ukclivecox Jun 10, 2023

agrski Jun 12, 2023

ukclivecox commented Jun 13, 2023

agrski left a comment

test: create initial Integration tests #4895

test: create initial Integration tests #4895

Conversation

ukclivecox commented Jun 7, 2023

review-notebook-app bot commented Jun 9, 2023

agrski left a comment

Choose a reason for hiding this comment

agrski Jun 9, 2023

Choose a reason for hiding this comment

ukclivecox Jun 10, 2023 • edited Loading

Choose a reason for hiding this comment

agrski Jun 12, 2023

Choose a reason for hiding this comment

agrski Jun 9, 2023

Choose a reason for hiding this comment

agrski Jun 9, 2023

Choose a reason for hiding this comment

ukclivecox Jun 10, 2023

Choose a reason for hiding this comment

agrski Jun 12, 2023

Choose a reason for hiding this comment

ukclivecox commented Jun 13, 2023

agrski left a comment

Choose a reason for hiding this comment

ukclivecox Jun 10, 2023 •

edited

Loading