-
Notifications
You must be signed in to change notification settings - Fork 827
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 SDK server HTTP API test #1079
Add SDK server HTTP API test #1079
Conversation
I am trying to avoid autogenerated code from including to this PR. So I generate
|
Build Failed 😱 Build Id: 12c7eadd-db95-4cd9-a1a2-ae8ff8b2d38d To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
05c73cd
to
9676408
Compare
Build Failed 😱 Build Id: 4603a107-eb07-4de1-a960-7a155c5f8c5f To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
9676408
to
fc0d9ae
Compare
Build Failed 😱 Build Id: ace3b3a2-ef72-45b9-8e63-0054141b5b9d To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
fc0d9ae
to
feb78e3
Compare
Build Succeeded 👏 Build Id: 957d39ed-fd87-4d32-b8eb-47a9d3e54843 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
feb78e3
to
dbdb640
Compare
/assign @roberthbailey |
Build Succeeded 👏 Build Id: a880bf40-bc36-4d32-8794-ad35adf1db44 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
dbdb640
to
de1259a
Compare
Build Succeeded 👏 Build Id: fe9e76c6-b5d7-407a-b9a4-ace613464d2c The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
de1259a
to
bb0d420
Compare
Build Succeeded 👏 Build Id: 31b66424-d8d3-429e-a561-a7dcbd7a58d6 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Can you explain why |
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'm really excited to have a conformance test for the rest apis. Thanks for putting together this PR.
build/includes/sdk.mk
Outdated
@@ -129,11 +133,12 @@ run-sdk-conformance-no-build: RANDOM := $(shell bash -c 'echo $$RANDOM') | |||
run-sdk-conformance-no-build: DELAY ?= $(shell bash -c "echo $$[ ($(RANDOM) % 5 ) + 1 ]") | |||
run-sdk-conformance-no-build: TESTS ?= ready,allocate,setlabel,setannotation,gameserver,health,shutdown,watch,reserve | |||
run-sdk-conformance-no-build: PORT ?= 59357 |
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.
If the other one is HTTP_PORT then this one should be GRPC_PORT.
if err != nil { | ||
log.Fatalf("Error in WatchGameServer: %v\n", err) | ||
} | ||
uid = gs.ObjectMeta.Uid |
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.
https://github.com/googleforgames/agones/blob/master/test/sdk/go/sdk-client-test.go sets the uid through a channel, which seems more thread safe.
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.
Used channel as in Go SDK test.
go func() { | ||
gs, _, err := cli.SDKApi.WatchGameServer(ctx) | ||
if err != nil { | ||
log.Fatalf("Error in WatchGameServer: %v\n", err) |
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 log.Fatal should only happen in the main goroutine. Can this pass the error back to the main goroutine instead of a uid to signal that it failed?
log.Fatalf("Could not SetLabel: %v\n", err) | ||
} | ||
|
||
time.Sleep(3 * time.Second) |
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.
there isn't a sleep between the other api calls, why one here?
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.
Maybe this wouldn't be necessary if you used a channel to pass back the uid (which would block).
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.
Yes, I removed this sleep
if err != nil { | ||
log.Fatalf("Could not SetAnnotation: %v\n", err) | ||
} | ||
time.Sleep(1 * time.Second) |
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.
why a sleep here?
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 is redundant, removed
First of all if we add this Go file we should also add multiple files generated by Swagger. This full import: |
Overall there is 69 errors in swagger generated code. Example of linting errors can be found here:
|
There is one more issue with Swagger or Need to find a way to change |
One more difference in that we receive
|
When I added new Result struct:
And switched from This leads us to the need of changing |
I have updated this PR and include a TODO which could be removed when Swagger json would be updated. |
Build Succeeded 👏 Build Id: ca6e68c5-50bb-4193-8c55-8f81c4a70d4c The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 0b679df4-5b94-49f9-bee8-a3dc2a20ac97 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 369f9a53-b67b-49df-8131-6fb59216e087 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
7cf1838
to
b24ec55
Compare
Build Succeeded 👏 Build Id: 558a9eb3-7331-429f-8fb4-9e7f79fc4716 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
b24ec55
to
5f3c1d7
Compare
Build Succeeded 👏 Build Id: 13b04f2e-77e6-4354-81c0-38ab1ba3bad2 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
|
||
func main() { | ||
log.Println("Client is starting") | ||
time.Sleep(1 * time.Second) |
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.
Is this sleep necessary? The waiting loop below should take care of startup timing differences between the test and the sdkserver.
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
|
||
ctx := context.Background() | ||
|
||
// Wait for SDK server to start (5 seconds) |
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.
Change 5 -> 30 to be consistent with the SDKs?
|
||
once := true | ||
go func() { | ||
gs, _, err := cli.SDKApi.WatchGameServer(ctx) |
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 looks like it only fires once. I see two ramifications:
- If the first time fails for some reason the test will fail. Should it be in a loop?
- The error flow shouldn't be indented
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 will look closely at your suggestions tomorrow.
WatchGameServer()
would not succeed currently, as it wait for closing the connection not line termination symbol (tracked here #1106 ). So I left this lines as is and they would actually work, when Swagger spec and generator would be fixed.
gs, _, err := cli.SDKApi.WatchGameServer(ctx) | ||
log.Printf("Watch response: %+v", gs) | ||
if err != nil { | ||
log.Printf("Error in WatchGameServer: %v\n", err) |
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.
return (or continue if in a loop) here to keep the rest of the control flow further left.
log.Fatalf("Could not SetLabel: %v\n", err) | ||
} | ||
|
||
// TODO: fix WatchGameServer() HTTP API Swagger definition and remove the following lines |
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.
The go routine above writes to c
as does this one. Can we delete this TODO and the below go routine? Or does the watch never fire?
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.
The watch function does not return, mentioned this in above comment, it is waiting for EOF, and "result". Opened a ticket for that.
5f3c1d7
to
774c80d
Compare
Build Succeeded 👏 Build Id: 0c43ac6a-a7a4-4501-bcda-0e57f9edea27 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
This PR now has a conflicting file. |
774c80d
to
a8d1b39
Compare
Build Succeeded 👏 Build Id: 36f5a30b-a163-407e-a9c6-69a79692ba7a The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Add conformance test for Rest API SDK server port 59358. Does not contain swagger pregenerated files. Switched Go file to other file extension, because it is not possible to exclude one file from GolangCI linter run.
a8d1b39
to
b4a0b13
Compare
Build Succeeded 👏 Build Id: 89301d9a-070b-447c-b22b-397d6d53ead1 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aLekSer, roberthbailey The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
New changes are detected. LGTM label has been removed. |
Build Succeeded 👏 Build Id: a55efbe6-2f81-4b17-aba6-41adbf7587bf The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Add conformance test for Rest API SDK server port 59358.
Does not contain swagger pregenerated files. Switched Go file to other
file extension, because it is not possible to exclude one file from
GolangCI linter run.
For #927