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

Add Reserve to Node.js SDK #955

Merged
merged 4 commits into from
Jul 30, 2019

Conversation

steven-supersolid
Copy link
Collaborator

@steven-supersolid steven-supersolid commented Jul 27, 2019

Add wrapper method
Add tests
Update docs
Add missing doc entry for Allocate

Part of #927

Update tests and docs
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: b593ddd7-08f9-41a5-b9bc-4659985031cb

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:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/955/head:pr_955 && git checkout pr_955
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-1d9cd98

@roberthbailey
Copy link
Member

Can you also update https://github.com/googleforgames/agones/blob/master/build/includes/sdk.mk#L142 to add the conformance test for the new function?

@steven-supersolid
Copy link
Collaborator Author

I've added but noticed the conformance test does not check anything, just executes methods. I'm not really familiar with this kind of test but perhaps better if the test is written more like the existing unit tests but using the running server instead of mocking, so an integration test.

If so I can create a separate PR which will use the current unit test system to test all methods of the SDK. It might also be useful to move this to the SDK folder so it can be easily run and updated during SDK development e.g. with npm run conformance.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 99a5c2db-9e74-4ce4-a7b9-61ac19c989e7

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@roberthbailey
Copy link
Member

I've added but noticed the conformance test does not check anything, just executes methods.

Interesting. I hadn't actually looked at them in detail, I just knew that the Go test was exercising different methods than the others. I guess maybe they are just validating that the methods have been implemented (e.g. that the SDK "conforms" to the proper set of functions) and we rely on individual SDK tests to make sure that the methods actually work properly?

@markmandel @aLekSer

@markmandel
Copy link
Collaborator

Yeah - the conformance test is to make sure the gRPC / REST calls get made -- and we get the results from the client that is expected.

We have e2e and unit results to test the actual results - but if we know the SDK clients conform to the API surface of the grpc / reset server, then we are fairly sure that the SDKs actually work.

Does that make sense?

@aLekSer
Copy link
Collaborator

aLekSer commented Jul 29, 2019

Actually we also check that we that SDK client able to read UID and CreationTimestamp from Watch and Get GameServer endpoints and set them properly as Label and Annotation. So we have a simple feedback mechanism here.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 5e1f0fe1-3e0b-4155-bcec-0abe8f5f215f

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@markmandel
Copy link
Collaborator

Weird. These are go tests that are failing. Lemme retry the test.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 75b0369a-66fa-42f4-b052-7530a9cc3870

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@markmandel
Copy link
Collaborator

Oh man, looks like some GitHub urls got moved under you. I'll look into getting a PR in place, otherwise this will block everything

@markmandel
Copy link
Collaborator

This should unblock once #961 is pushed through.

Copy link
Collaborator

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

LGTM!

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 05c98efa-058f-451f-84fa-8ed079f2455a

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:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/955/head:pr_955 && git checkout pr_955
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-1b20116

@markmandel markmandel merged commit 565431f into googleforgames:master Jul 30, 2019
@markmandel markmandel added area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/feature New features for Agones labels Jul 30, 2019
@markmandel markmandel added this to the 0.12.0 milestone Jul 30, 2019
@steven-supersolid steven-supersolid deleted the nodejs.reserve branch March 14, 2020 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/feature New features for Agones
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants