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.

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

impl for integration test #292

Merged

Conversation

SamYuan1990
Copy link
Collaborator

Details for this PR
e2e folder with end to end testing, will run with two models, base on environment kepler_address.

build kepler during test process and run it targets on local.
run test case targets on cluster after cluster port forwarding.

Signed-off-by: Sam Yuan yy19902439@126.com

@SamYuan1990
Copy link
Collaborator Author

redo for #275

Signed-off-by: Sam Yuan <yy19902439@126.com>
@SamYuan1990
Copy link
Collaborator Author

a part of #162

Copy link
Collaborator

@marceloamaral marceloamaral left a comment

Choose a reason for hiding this comment

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

I think the tests should be in a folder called test/
So, you will have test/e2e/

@marceloamaral
Copy link
Collaborator

will run with two models, base on environment kepler_address.

Which models? Power models?

if !ok {
address = "localhost:8888"
cmd := exec.Command(keplerBin)
keplerSession, err = gexec.Start(cmd, nil, nil)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will the test start Kepler alone or with the other components, like the Model Server?

If it is only Kepler, what is the integration test part?

I am not saying that this test here is not important, but it might not be integration test as the title suggest...

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the case it is not integration test, this is definitely a functional test that we need to have

@sunya-ch
Copy link
Collaborator

sunya-ch commented Oct 12, 2022

As I think this PR relates to kepler-model-server and kepler-estimator integration test and I don't want to duplicate the work here, please excuse me to put some of my thought here.

First of all, here is the integration test flow for kepler-estimator connecting to kepler-model-server with kind cluster: https://github.com/sustainable-computing-io/kepler-estimator/blob/main/.github/workflows/integration_test.yaml.

I think we can do a similar thing with kepler integration test but, for simplicity, I think we should let kepler defines its configurations from configmap resource rather than changing the main deployment environment or command argument (please check PR #284).

Then, we can easily configure ModelConfig implemented by this PR #225, in the same way as I did with configmap of kepler-estimator to whether to use kepler-model-server, kepler-estimator or not.

We can create test cases including

  1. with only kepler-esitmator sidecar coming with initial models
  2. with only kepler-model-server
  3. with kepler-esitmator without initial models and kepler-model-server
    and so on.

@@ -86,6 +86,8 @@ github.com/onsi/ginkgo/types
## explicit; go 1.18
github.com/onsi/gomega
github.com/onsi/gomega/format
github.com/onsi/gomega/gbytes
Copy link
Contributor

Choose a reason for hiding this comment

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

why not change in go.mod and go.sum?

@rootfs
Copy link
Contributor

rootfs commented Oct 13, 2022

@SamYuan1990 I just have a question on go mod.

@sunya-ch sounds good! After this PR, can you create an issue/request?

@marceloamaral @sunya-ch anything else is needed for this PR? It'll be great to have an initial integration test in place and add more in the future.

Copy link
Collaborator

@marceloamaral marceloamaral left a comment

Choose a reason for hiding this comment

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

/lgtm

This is a good start.

Even though I think it is a functional test, so that the workflow file name would be different... I am ok to keep like that for now....

@rootfs rootfs merged commit a2f9796 into sustainable-computing-io:main Oct 14, 2022
@sunya-ch
Copy link
Collaborator

@SamYuan1990 I just have a question on go mod.

@sunya-ch sounds good! After this PR, can you create an issue/request?

@marceloamaral @sunya-ch anything else is needed for this PR? It'll be great to have an initial integration test in place and add more in the future.

Sure. I can push the PR for the deployment CI test later after all components is clearly integrated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants