-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add test for scaling up a function #14
Conversation
4646983
to
75bfda0
Compare
This adds autoscaling test for the usecases when there are no scaling limits configured Signed-off-by: Ivana Yovcheva (VMware) <iyovcheva@vmware.com>
75bfda0
to
3d55cee
Compare
|
||
invoke(t, functionRequest.Service, emptyQueryString, http.StatusOK) | ||
|
||
err := scale(t, functionRequest.Service, "10") |
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.
I think 10 is too high for a CI machine, just going from 1 to two would be fine.
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.
Fixed in PR#24.
|
||
} | ||
|
||
func scale(t *testing.T, fnName string, replicas string) error { |
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.
It'd be better not to pass testing.T and then deal with any testing stuff in the caller.
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.
Fixed in PR#24.
I noticed that other functions have testing.T parameters. Examples in deploy_test.go are
- line 267, deploy()
- 283, list()
- 308, get()
That might explain why @ivanayov did something similar.
return fmt.Errorf("Failed to POST request %s to endpoint %s. Error: %t", request, endpoint, err) | ||
} | ||
|
||
if res.StatusCode != 200 && res.StatusCode != 202 { |
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.
Rather than integers use http.StatusAccepted
etc
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.
Fixed in PR#24.
} | ||
|
||
if res.StatusCode != 200 && res.StatusCode != 202 { | ||
return fmt.Errorf("Failed scaling function on endpoint: %s. %d %s", endpoint, res.StatusCode, res.Body) |
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.
Errors start with a lowercase letter
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.
Fixed in PR#24.
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.
This needs a few small changes before being merged. Let us know if you can pick it up, or whether we should find someone else to take it over.
I can work on finishing this one. |
Thank you 🙏 |
I found an example of @Waterdrips fixing someone else's fork here: openfaas/openfaas-cloud#531 It seems like the original PR was closed and a new one was created. I'll attempt something similar. @alexellis, unless you had another way in mind, I think this PR can be closed for PR #24. |
You could add "Closes #14" in the PR Description #24 (comment) |
This was resolved in #29 |
This adds autoscaling test for the usecases when there are no
scaling limits configured
Signed-off-by: Ivana Yovcheva (VMware) iyovcheva@vmware.com
Resolves #13