agones icon indicating copy to clipboard operation
agones copied to clipboard

First test for the agones unity client sdk

Open aallbrig opened this issue 1 year ago • 1 comments

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking /kind bug /kind cleanup /kind documentation /kind feature /kind hotfix /kind release

What this PR does / Why we need it: Following instructions proposed in https://github.com/googleforgames/agones/pull/3886 I set up a local environment to contribute to agones unity client sdk.

I noticed there wasn't a test. This adds a first test. I don't think it's a good test so please offer up what might make for a better first test.

Which issue(s) this PR fixes:

Closes #

Special notes for your reviewer:

aallbrig avatar Jun 29 '24 19:06 aallbrig

Build Succeeded :clap:

Build Id: 5078706a-0651-4905-aa15-cae53be14e47

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):

  • https://5c54ed5-dot-preview-dot-agones-images.appspot.com/

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/3887/head:pr_3887 && git checkout pr_3887
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.42.0-dev-5c54ed5-amd64

agones-bot avatar Jun 29 '24 20:06 agones-bot

Build Succeeded :clap:

Build Id: 9ff063dc-f993-481a-a6dd-a505f55b3f6a

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):

  • https://19512c6-dot-preview-dot-agones-images.appspot.com/

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/3887/head:pr_3887 && git checkout pr_3887
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.42.0-dev-19512c6-amd64

agones-bot avatar Jul 14 '24 21:07 agones-bot

Noob question -- what are the steps to run the tests? Are we able to run the tests similar to vanilla csharp (csharp SDK tests have the non-obvious steps of 1. navigating to agones/build 2. running make sdk-shell-csharp 3. once in the C# shell navigating to the test folder /go/src/agones.dev/agones/sdks/csharp/test 4. using standard dotnet commands such as dotnet test.), or does this require a Unity editor?

igooch avatar Jul 15 '24 06:07 igooch

Noob question -- what are the steps to run the tests? Are we able to run the tests similar to vanilla csharp (csharp SDK tests have the non-obvious steps of 1. navigating to agones/build 2. running make sdk-shell-csharp 3. once in the C# shell navigating to the test folder /go/src/agones.dev/agones/sdks/csharp/test 4. using standard dotnet commands such as dotnet test.), or does this require a Unity editor?

For the tests setup I added in #3883, I added a README that might help answer this for you. It's at the very bottom of the PR. My understanding is that it's basically impossible to run these tests without the unity editor. Even the common CI setups for Unity are just running a dockerized editor.

ZeroParticle avatar Jul 15 '24 21:07 ZeroParticle

My understanding is that it's basically impossible to run these tests without the unity editor. Even the common CI setups for Unity are just running a dockerized editor.

Would it makes sense to open up a new issue for creating some sort of pipeline for running the Unity tests? Possibly CloudBuild could take in a Unity license as a Secret and run the tests in the dockerized editor mode?

igooch avatar Jul 15 '24 21:07 igooch

@igooch You're definitely right that developing unity packages is not intuitive. I wrote documentation in this PR (https://github.com/googleforgames/agones/pull/3886) for how to set up a testing environment that's common. I've used this setup when contributing to other unity open source projects like Mirror. You can follow the instructions in that PR to set up to then develop a unity package.

Should I merge that PR into this one, so documentation exists along with this test?

aallbrig avatar Jul 16 '24 02:07 aallbrig

@igooch I can help with using game CI's docker container to run tests. I've used that container image before to cross platform test & build in parallel since I didn't like serially builds in unity editor.

There's a little setup to do but I've scripted its use out before. Would need to review this project's CI more to understand how to introduce a new testing step (guidance is welcome and appreciated!)

Unity docker usage seems outside the scope of this PR, maybe, since it's a test suite agnostic enhancement? WDYT?

aallbrig avatar Jul 16 '24 02:07 aallbrig

Build Succeeded :clap:

Build Id: b324d10c-f98c-4b56-ac8d-e0ab706f6e3c

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):

  • https://48eaee5-dot-preview-dot-agones-images.appspot.com/

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/3887/head:pr_3887 && git checkout pr_3887
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.42.0-dev-48eaee5-amd64

agones-bot avatar Jul 16 '24 16:07 agones-bot

Build Succeeded :clap:

Build Id: a2c8734d-c23a-468c-9cd5-b2d57fa26499

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):

  • https://af604a3-dot-preview-dot-agones-images.appspot.com/

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/3887/head:pr_3887 && git checkout pr_3887
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.43.0-dev-af604a3-amd64

agones-bot avatar Jul 18 '24 19:07 agones-bot

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions[bot] avatar Jul 19 '24 23:07 github-actions[bot]

Merged in the contents of https://github.com/googleforgames/agones/pull/3886 into this PR, to provide instructions for this test.

In order to simplify the setup instructions I created a new Unity project in test/sdk/unity, which is very much inspired by @ZeroParticle's https://github.com/googleforgames/agones/pull/3883. The difference between this implementation and @ZeroParticle's is that I keep the test co-located with the Agones Unity SDK custom package which this new test unity project pulls in (relative path in Packages/manifest.json), whereas @ZeroParticle has their test in the Unity project which isn't standard to typical Unity Custom Packages (See Unity Custom Package Layout).

The PR is still WIP; now that I can have some guarantees with the Unity Project I can install some additional testing libraries more easily. Instead of having sdks/unity/Tests/TestingEnvironment/MockAgonesSdkServer.cs I could use a 3rd party library to stub out UnityWebRequest for tests.

aallbrig avatar Jul 19 '24 23:07 aallbrig

Build Succeeded :clap:

Build Id: 0e6997f0-780b-45dd-8851-e7f4fc6a43a4

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):

  • https://4e8caa0-dot-preview-dot-agones-images.appspot.com/

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/3887/head:pr_3887 && git checkout pr_3887
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.43.0-dev-4e8caa0-amd64

agones-bot avatar Jul 20 '24 01:07 agones-bot

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions[bot] avatar Jul 20 '24 17:07 github-actions[bot]

@igooch @markmandel et all This PR is ready to be reviewed! Features:

  • New Unity project created at test/sdk/unity that's already pre-configured to pull in local Agones Unity SDK locally (see test/sdk/unity/Package/manifest.json so see how). This serves as the host Unity project, which is required for Unity Custom Package development.
  • Instructions added on how to develop the Agones Unity SDK added so that developers know how to do things like add the test/sdk/unity project to their Unity Hub, and how development will work.
  • A single test that spies to ensure AgonesSdk component makes request to the Agones SDK Server. I want to add more tests but hope for feedback. View sdks/unity/Tests/Runtime/PlayMode/AgonesSdkComplianceTests.cs to view the test!
Screenshot 2024-07-20 at 1 42 41 PM

The PR is well over 1000 LoC primarily due to having a new Unity project contained in the changeset. See below to see that a majority of this PR's LoC come from default files created when initializing a new Unity project.

$ git diff --numstat main...agones-unity-client-sdk-initialize-test | sort -k1,1nr

671     0       test/sdk/unity/ProjectSettings/ProjectSettings.asset
322     0       test/sdk/unity/Packages/packages-lock.json
322     0       test/sdk/unity/ProjectSettings/QualitySettings.asset
316     0       test/sdk/unity/Assets/Main.unity
296     0       test/sdk/unity/ProjectSettings/InputManager.asset
121     0       test/sdk/unity/ProjectSettings/SceneTemplateSettings.json
93      0       test/sdk/unity/ProjectSettings/NavMeshAreas.asset
72      0       test/sdk/unity/.gitignore
67      0       test/sdk/unity/ProjectSettings/GraphicsSettings.asset
50      0       sdks/unity/Tests/Runtime/PlayMode/AgonesSdkComplianceTests.cs
48      0       test/sdk/unity/ProjectSettings/Physics2DSettings.asset
47      0       test/sdk/unity/ProjectSettings/EditorSettings.asset
44      0       test/sdk/unity/Packages/manifest.json
43      0       test/sdk/unity/ProjectSettings/TagManager.asset
40      0       test/sdk/unity/ProjectSettings/DynamicsManager.asset
36      0       test/sdk/unity/ProjectSettings/PackageManagerSettings.asset
36      0       test/sdk/unity/ProjectSettings/UnityConnectSettings.asset
35      0       test/sdk/unity/ProjectSettings/MemorySettings.asset
25      0       sdks/unity/Tests/TestingEnvironment/SpyRequestSender.cs
23      0       sdks/unity/Tests/Runtime/PlayMode/Tests.Runtime.Agones.asmdef
20      0       test/sdk/unity/ProjectSettings/AudioManager.asset
18      0       test/sdk/unity/ProjectSettings/VFXManager.asset
17      12      sdks/unity/AgonesSdk.cs
14      0       sdks/README.md
14      0       sdks/unity/README.md
12      2       sdks/unity/Agones.asmdef
10      0       sdks/unity/IRequestSender.cs
9       0       test/sdk/unity/ProjectSettings/TimeManager.asset
8       0       sdks/unity/Tests/Runtime/PlayMode.meta
8       0       test/sdk/unity/ProjectSettings/EditorBuildSettings.asset
8       0       test/sdk/unity/ProjectSettings/VersionControlSettings.asset
7       0       sdks/unity/README.md.meta
7       0       test/sdk/unity/Assets/Main.unity.meta
7       0       test/sdk/unity/ProjectSettings/PresetManager.asset
6       0       test/sdk/unity/ProjectSettings/ClusterInputManager.asset
5       0       test/sdk/unity/README.md
3       0       sdks/unity/IRequestSender.cs.meta
3       0       sdks/unity/Tests.meta
3       0       sdks/unity/Tests/Runtime.meta
3       0       sdks/unity/Tests/Runtime/PlayMode/AgonesSdkComplianceTests.cs.meta
3       0       sdks/unity/Tests/Runtime/PlayMode/Tests.Runtime.Agones.asmdef.meta
3       0       sdks/unity/Tests/TestingEnvironment.meta
3       0       sdks/unity/Tests/TestingEnvironment/SpyRequestSender.cs.meta
2       0       README.md
2       0       test/sdk/unity/ProjectSettings/ProjectVersion.txt

Without the .asset, .meta, package-lock.json, and .unity files this change set would probably be considered reasonable.

$ git diff --numstat main...agones-unity-client-sdk-initialize-test | grep -v '!*.asset\|!*.meta\|!*-lock.json\|!*\.unity' | sort -k1,1nr

121     0       test/sdk/unity/ProjectSettings/SceneTemplateSettings.json
72      0       test/sdk/unity/.gitignore
50      0       sdks/unity/Tests/Runtime/PlayMode/AgonesSdkComplianceTests.cs
44      0       test/sdk/unity/Packages/manifest.json
25      0       sdks/unity/Tests/TestingEnvironment/SpyRequestSender.cs
23      0       sdks/unity/Tests/Runtime/PlayMode/Tests.Runtime.Agones.asmdef
17      12      sdks/unity/AgonesSdk.cs
14      0       sdks/README.md
14      0       sdks/unity/README.md
12      2       sdks/unity/Agones.asmdef
10      0       sdks/unity/IRequestSender.cs
5       0       test/sdk/unity/README.md
2       0       README.md
2       0       test/sdk/unity/ProjectSettings/ProjectVersion.txt

aallbrig avatar Jul 20 '24 18:07 aallbrig

Build Succeeded :clap:

Build Id: eb16f452-33c0-47dd-8d0b-9b68bef4f291

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):

  • https://4ac32fa-dot-preview-dot-agones-images.appspot.com/

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/3887/head:pr_3887 && git checkout pr_3887
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.43.0-dev-4ac32fa-amd64

agones-bot avatar Jul 20 '24 18:07 agones-bot

@zmerlynn @igooch @markmandel tagging for some review. Thank you in advance!

aallbrig avatar Jul 23 '24 01:07 aallbrig

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions[bot] avatar Jul 23 '24 11:07 github-actions[bot]

Build Succeeded :clap:

Build Id: a8a38947-9a0f-40c2-835e-164b4f2dd94a

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):

  • https://b48b315-dot-preview-dot-agones-images.appspot.com/

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/3887/head:pr_3887 && git checkout pr_3887
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.43.0-dev-b48b315-amd64

agones-bot avatar Jul 23 '24 12:07 agones-bot

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions[bot] avatar Jul 23 '24 13:07 github-actions[bot]

@markmandel I added an apache header to all added C# code in this PR. I also added it to sdks/unity/model/StatusAddresses.cs which is a class that pre-exists this PR but I found it was missing an apache header.

I debated if I should mark the year 2019 or 2024; I opted for 2024.

aallbrig avatar Jul 23 '24 13:07 aallbrig

Build Succeeded :clap:

Build Id: d103deee-ec55-427d-8f3a-09e9f2f2cbd6

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):

  • https://e0ec4fa-dot-preview-dot-agones-images.appspot.com/

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/3887/head:pr_3887 && git checkout pr_3887
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.43.0-dev-e0ec4fa-amd64

agones-bot avatar Jul 23 '24 14:07 agones-bot

Happy to update my PR #3883 to use this project if that's what we go for. The tests in my project run against a local SDK server rather than a mock. That seems complimentary to what you have here, so I figure we will want to keep it around rather than replace it.

ZeroParticle avatar Jul 23 '24 18:07 ZeroParticle

@ZeroParticle Thanks for weighing in! Yes I think adding tests in the style of this PR is better than in the style of your PR. The style of this PR has the tests contained within the package files which is more expected for a custom unity package.

I'm hoping @markmandel (and/or other(s)) review/merge this PR soon which will enable both of us to evolve this base code.

aallbrig avatar Jul 23 '24 18:07 aallbrig

@zmerlynn @igooch @markmandel tagging for another round of review. Thank you again in advance!

aallbrig avatar Jul 24 '24 01:07 aallbrig

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions[bot] avatar Jul 24 '24 18:07 github-actions[bot]

Build Succeeded :clap:

Build Id: 236d2f3a-47a4-4f8d-b91e-3f44b53c18f2

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):

  • https://3f6648f-dot-preview-dot-agones-images.appspot.com/

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/3887/head:pr_3887 && git checkout pr_3887
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.43.0-dev-3f6648f-amd64

agones-bot avatar Jul 24 '24 19:07 agones-bot

Build Succeeded :clap:

Build Id: 368d740a-cdb0-4269-bcb7-864b648876e7

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):

  • https://3f316b7-dot-preview-dot-agones-images.appspot.com/

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/3887/head:pr_3887 && git checkout pr_3887
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.43.0-dev-3f316b7-amd64

agones-bot avatar Jul 25 '24 18:07 agones-bot

@ZeroParticle @aallbrig - @markmandel and I were chatting. We don't have a great way to test this right now, unfortunately. @ZeroParticle, if I review this, would you be up for ad hoc testing and seeing if it looks reasonable? @aallbrig - same question for other Unity PRs as they come in.

Basically, if y'all are willing, we're happy to review and then gradually let y'all be the Unity maintainers, but we may need help on the testing front.

zmerlynn avatar Jul 25 '24 20:07 zmerlynn

Build Succeeded :clap:

Build Id: 9617c1de-a0d0-4f9b-aecb-8f12a1a9e185

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):

  • https://e6eee46-dot-preview-dot-agones-images.appspot.com/

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/3887/head:pr_3887 && git checkout pr_3887
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.43.0-dev-e6eee46-amd64

agones-bot avatar Jul 25 '24 22:07 agones-bot

@zmerlynn I would be happy to help with the Agones Unity SDK maintenance and development. I think @ZeroParticle and I can help with testing and other project needs.

I think we both agree this PR will serve as a good base PR to build more tests for the Agones Unity SDK. Once merged, @ZeroParticle's AgonesSDK additions (+tests) can move under the Agones Custom Unity Package and I can help by code review + checking out the branch code on my machine.

I'll try to think up a way to make testing an easier to prove out for a broader audience.

aallbrig avatar Jul 26 '24 16:07 aallbrig