Skip to content

Conversation

EmandM
Copy link
Collaborator

@EmandM EmandM commented Oct 10, 2025

Purpose of this PR

I've spent too much time staring at yamato over #3647 . Decided to see if we can tweak our PR trigger to be more efficient.

Jira ticket

n/a

Changelog

  • Removed ::GPU from the agent.type for desktop_standalone_build, desktop_standalone_test and cmb_service_standalone_test. All of these jobs disable the GPU so they don't need a machine with a GPU (gpu machines are much rarer)
  • Swapped the PR trigger to run desktop_standalone_test_testproject_ubuntu_il2cpp_trunk rather than desktop_standalone_test_testproject_win_il2cpp_6000.0 that way the desktop_standalone_test and the cmb_service_standalone_test can use the same desktop_standalone_build step rather than needing two different build steps
    • This does mean we no longer run PR tests on ubuntu_6000.0.
    • I think this is fine as mac and windows are by far the most used platforms.
    • We will still test ubuntu_6000.0 on the nightly and weekly builds.
  • Defined a smaller_flavor field in test_platforms.desktop to use with lighter weight jobs (package_pack, package_tests, project_pack). I chose which jobs to use a smaller VM from looking at the machine usage stats on bokken (like I say, I stared at yamato TOO MUCH this week).
  • Updated upm-ci to UnifiedTestRunner for the project_pack and project_test jobs.

Documentation

  • No documentation changes or additions were necessary.

Testing & QA (How your changes can be verified during release Playtest)

No testing needed.

Functional Testing

Manual testing :

  • Manual testing done

Automated tests:

  • Covered by existing automated tests
  • Covered by new automated tests

Does the change require QA team to:

  • Review automated tests?
  • Execute manual tests?
  • Provide feedback about the PR?

If any boxes above are checked the QA team will be automatically added as a PR reviewer.

Backports

Will backport to 1.x because faster tests makes for happier devs.

@EmandM EmandM requested review from a team and NoelStephensUnity as code owners October 10, 2025 22:54
@EmandM EmandM added the port:1.x-needed This issue needs to be ported to 1.X branch label Oct 10, 2025
… into chore/yamato-tweaks

Add update of NetworkSceneManagerEventNotifications for cmb to test the whole PR trigger
@EmandM EmandM requested a review from a team as a code owner October 10, 2025 23:08
vetting_test:
name: NGO - Vetting Test (Win, {{editor}} LTS)
agent: { type: Unity::VM, flavor: b1.large, image: package-ci/win11:v4 }
agent: { type: Unity::VM, flavor: b1.medium, image: package-ci/win11:v4 }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair, to be hones we could even try with small since vetting test is analyzing the PR for the release (API check etc) so maybe nothing really memory expensive is being run

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was looking at the CPU usage and the vetting test does end up using a fair amount of CPU so I left it at medium
(Here's the machine usage for this job). We could try it on small and see what it looks like?

dependencies:
- .yamato/project-pack.yml#project_pack_-_{{ project.name }}_{{ platform.name }}
# - .yamato/project-pack.yml#project_pack_-_{{ project.name }}_{{ platform.name }}
- .yamato/package-pack.yml#package_pack_-_ngo_{{ platform.name }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this work? (packing package instead of project and all its packages)

Let's make sure to validate that this test passes (this is the same question per each "project related" test)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you look at the logs of the project_pack job, it only packed our package anyway 🙃

- npm install upm-ci-utils@stable -g --registry https://artifactory.prd.cds.internal.unity3d.com/artifactory/api/npm/upm-npm # upm-ci is not preinstalled on the image so we need to download it
- unity-downloader-cli --fast --wait -u {{ editor }} -c Editor {% if platform.name == "mac" %} --arch arm64 {% endif %} # For macOS we use ARM64 models. Installing basic editor for tests execution
- upm-ci project test -u {{ editor }} --project-path {{ project.path }} --type project-tests --extra-utr-arg="--reruncount=1 --clean-library-on-rerun" # project tests execution via upm-ci
- UnifiedTestRunner --testproject={{ project.path }} --suite=editor --artifacts-path=test-results --editor-location=.Editor --rerun-strategy=Test --retry=1 --timeout=1800
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use --suite=editor --suite=playmode so both kind of tests are being run.

Don't we need to specify the editor in the command? (I don't have perfect memory for those configurations so some questions may seem basic)

I think rerun-strategy is something new since I don't remember seeing it before

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

--rerun-strategy will let us only rerun the tests that failed rather than the whole suite!. But only running the test that failed only works when you run the two suites separately for some reason.

In these logs for package_tests you can see that ValidateUrlsAreValid was run twice and the other tests are only run once.

{% endif %}
commands:
- unity-downloader-cli -u {{ editor }} -c Editor --wait {% if platform.name == "mac" %} --arch arm64 {% endif %} # For macOS we use ARM64 models. Installing basic editor
- UnifiedTestRunner --suite=editor --suite=playmode --testproject={{ project.path }} --editor-location=.Editor --timeout=3600 --artifacts-path=artifacts --extra-editor-arg=-assemblyNames --extra-editor-arg=Unity.NetCode.* --extra-editor-arg=-testCategory --extra-editor-arg=Performance --extra-editor-arg=-enablePackageManagerTraces --reruncount=1 --clean-library-on-rerun --dontreportperformancedata
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo? I know there is some issue with regeneration sometimes when it changes this param. Same question to other scripts where it changed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm putting it back! I thought it was this argument that made it rerun all the tests when one failed but it's not this

# Run tests for Android devices
- |
set ANDROID_DEVICE_CONNECTION=%BOKKEN_DEVICE_IP%
UnifiedTestRunner --suite=playmode --platform={{ platform.standalone }} --artifacts-path=test-results --player-load-path=build/players --testproject={{ project.path }} --editor-location=.Editor --player-connection-ip=%BOKKEN_HOST_IP% --fail-on-assert --reruncount=1 --clean-library-on-rerun --timeout=3600
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why to remove it? Same question for other instances

# Run standalone test. We run it only on Ubuntu since it's the fastest machine, and it was noted that for example distribution on macOS is taking 40m since we switched to Apple Silicon
# Coverage on other standalone machines is present in Nightly job so it's enough to not run all of them for PRs
- .yamato/desktop-standalone-tests.yml#desktop_standalone_test_testproject_win_il2cpp_6000.0
- .yamato/desktop-standalone-tests.yml#desktop_standalone_test_testproject_ubuntu_il2cpp_trunk
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason why to run this on trunk instead of the "minimal supported editor"?

Copy link
Collaborator Author

@EmandM EmandM Oct 13, 2025

Choose a reason for hiding this comment

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

It re-uses the same standalone-build step as the CMB tests so we don't need to distribute and run two separate standalone-build steps

# ensure the code is running to our current standards
- .yamato/project-standards.yml#standards_ubuntu_testproject_trunk
# Run API validation to early-detect all new APIs that would force us to release new minor version of the package. Note that for this to work the package version in package.json must correspond to "actual package state" which means that it should be higher than last released version
- .yamato/vetting-test.yml#vetting_test
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason why to remove it?

The configuration is a bit tricky in the flow but those jobs run before any other job gets executed so we take resources only if we don't have any basic mistakes. Vetting test will fail when for example something gets flagged with new API

Note that if this will pass and we declare it as dependency later (in the trigger) then Yamato will just reuse the result of this job so we are not wasting any resources/time here

Copy link
Collaborator Author

@EmandM EmandM Oct 13, 2025

Choose a reason for hiding this comment

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

The Vetting test was generally taking longer to run than the standards test (Standards check is normally 5m, Vetting test is normally 9m). So far, it's also been some thing that fails very rarely.

I also moved the vetting test to a smaller machine than before so it'll get distributed faster. That means the job was running a bit slower again. I was finding it a bit slow to be a quick initial check, and seeing as it shouldn't fail very often, I was thinking it made more sense to run not as a "quick" check.

@svc-netcode-sdk svc-netcode-sdk requested a review from a team October 13, 2025 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

port:1.x-needed This issue needs to be ported to 1.X branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants