-
Notifications
You must be signed in to change notification settings - Fork 329
CHANGE: Migrate ci to recipe engine #2226
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
base: develop
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2226 +/- ##
===========================================
+ Coverage 68.14% 76.70% +8.56%
===========================================
Files 367 465 +98
Lines 53685 87919 +34234
===========================================
+ Hits 36585 67442 +30857
- Misses 17100 20477 +3377 Flags with carried forward coverage won't be shown. Click here to find out more. see 100 files with indirect coverage changes 🚀 New features to boost your workflow:
|
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.
Thanks for doing this! While this is now a bunch more files flying around than before, it is going to be easier to keep alive without everyone bolting every new thing onto that one yml file :D
Only two small comments from my side.
.WithRerun(1, true) | ||
.WithPerformanceDataReporting(true) | ||
.WithPerformanceProject("InputSystem") | ||
.WithExtraArgs("--report-performance-data --performance-project-id=InputSystem") |
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 line produces "--report-performance-data --performance-project-id=InputSystem" in the yml cmd line a 2nd time, since the two .Withs above it already say the same thing.
Looks fine for the other performance test recipes.
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.
Good catch!
.WithDependencies(allMobileFunctionalTests) | ||
.WithDependencies(allTvOSFunctionalBuildJobs), | ||
|
||
JobBuilder.Create("All Functional Tests") |
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 a trigger could be added for "all functional editor jobs for trunk" to run during development to save some clicks and have a quick CI turnaround before the PR stage? But I'll leave that up to @jfreire-unity to decide. Just a thought.
… and regenerate CI
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.
Looks like a good improvement, so thanks for this!
I added some comments to avoid code repetition and questions regarding matching the current functionality.
Also, I think this area would benefit from having a README in case someone in the team needs to do a CI fix. I think a lot of us are not aware of how these CI Recipes so I'll try to book a meeting next week.
I'm not sure how we will "deploy" this. Are we able to test this on demand before landing the PR to make sure we have 1:1 matching with the current jobs that run?
My last comment/nitpick is about the size of the PR. We could have probably done this in smaller chunks so that we wouldn't have so many changes landing. Also would have been easier to review.
Nevertheless, this is great work, and thanks for improving our CI workflow! 🚀 🙏
Tools/CI/Recipes/InputBaseRecipe.cs
Outdated
|
||
namespace InputSystem.Cookbook.Recipes; | ||
|
||
public abstract class InputBaseRecipe: RecipeBase |
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.
Nit pick: We're in the InputSystem.Cookbook.Recipes
so I would think we don't need Input
prefix for inputBaseRecipe
, InputMobileBaseRecipe
. But this is just my preference and I don't know if without those prefixes, those classes exist elsewhere. You will likely be more into this area so I trust your judgement.
if (platform.System == SystemType.Android && jobName.Contains("il2cpp")) | ||
{ | ||
job.WithCommands(UtrCommand.Run(platform.System, b => b | ||
.WithTestProject($"{ProjectPath}") | ||
.WithEditor(".Editor") | ||
.WithSuite(UtrTestSuiteType.Playmode) | ||
.WithPlatform(platform.System) | ||
.WithCategory("!Performance") | ||
.WithScriptingBackend(ScriptingBackendType.Il2Cpp) | ||
.WithExtraArgs("--clean-library") | ||
.WithRerun(1, true) | ||
.WithBuildOnly() | ||
.WithPlayerSavePath("build/players") | ||
.WithArtifacts("build/logs"))); | ||
} | ||
else if(platform.System == SystemType.TvOS) | ||
{ | ||
job.WithCommands(UtrCommand.Run(platform.System, b => b | ||
.WithTestProject($"{ProjectPath}") | ||
.WithEditor(".Editor") | ||
.WithSuite(UtrTestSuiteType.Playmode) | ||
.WithCategory("!Performance") | ||
.WithExtraArgs("--platform=tvOS --clean-library") | ||
.WithRerun(1, true) | ||
.WithBuildOnly() | ||
.WithPlayerSavePath("build/players") | ||
.WithArtifacts("build/logs"))); | ||
} | ||
else | ||
{ | ||
job.WithCommands(UtrCommand.Run(platform.System, b => b | ||
.WithTestProject($"{ProjectPath}") | ||
.WithEditor(".Editor") | ||
.WithSuite(UtrTestSuiteType.Playmode) | ||
.WithPlatform(platform.System) | ||
.WithCategory("!Performance") | ||
.WithExtraArgs("--clean-library") | ||
.WithRerun(1, true) | ||
.WithBuildOnly() | ||
.WithPlayerSavePath("build/players") | ||
.WithArtifacts("build/logs"))); | ||
} |
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 see some repeated method calls such as WithArtifacts
, WithPlayersSavePath
, etc. I wonder if we can make all of the repetition go away so that it's clearer of what's different bewteeen the jobs.
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.
Also, you can take this comment to all the other places we are creating jobs. I don't know it the builder pattern needs to follow a specific order as well. Still, I think at least adding the methods that are different depending on the condition we check for would be beneficial to all maintainers.
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 have done some refactoring to reduce the duplication. Please re-review.
job.WithCommands(UtrCommand.Run(platform.System, b => b | ||
.WithTestProject($"{ProjectPath}") | ||
.WithEditor(".Editor") | ||
.WithSuite(UtrTestSuiteType.Playmode) | ||
.WithCategory("!Performance") | ||
.WithExtraArgs("--platform=tvOS --clean-library") | ||
.WithRerun(1, true) | ||
.WithBuildOnly() | ||
.WithPlayerSavePath("build/players") | ||
.WithArtifacts("build/logs"))); |
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 don't see a WithPlatform
here. Is it expected?
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.
"WithPlatform" doesn't work with tvOS. Instead I used --platform=tvOS
directly.
if (platform.System == SystemType.IOS && float.Parse(unityVersion) > 6000.2f) | ||
job.WithEnvironmentVariable("UNITY_HANDLEUIINTERRUPTIONS", 1); |
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 is this only done for 6.2 and forward? Maybe some comments explaining this would help understand its need.
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.
Have added some comments
dotnet run --project Tools/CI/InputSystem.Cookbook.csproj |
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.
When should this be run? Could we actually add a README.md to Tools/CI
for us to know how it is setup and what to run when we make changes?
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.
We need to run regenerate.sh[bat] when any of following things happen:
- Changes made to InputSystem.Cookbook project
- An unity version reaches EOL
- Trunk version has changed which also means a new version has branched off from Trunk
- Minimum Editor version of Input package has changed
- Wrench version is updated
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.
Thanks for doing this! Approving to not leave this lingering longer. No blockers for me.
Description
This PR migrates CI to use Recipe Engine.
In Input CI, tests fall into two categories:
functional tests
andperformance tests
.For each category, tests run on the following Runtime platforms:
Windows
,Ubuntu
andMacOS
TvOS
,iOS
andAndroid
. For Android, we cover bothmono
andil2cpp
backends.In the migration, I created four recipes for both
Functional
andPerformance
tests.For functional tests, they are:
EditorFunctionalTests
StandaloneFunctionalTests
StandaloneIl2CppFunctionalTests
MobileFunctionalTests
For performance tests, there are four equivalents:
EditorPerformanceTests
StandalonePerformanceTests
StandaloneIl2CppPerformanceTests
MobilePerformanceTests
For editor and standalone tests recipes, the tests will run on
WrenchPackage.DefaultEditorPlatforms
which are Windows, Mac and Ubuntu. That's what we want.For mobile tests recipe, I added the following platforms in
InputSystemSettings.cs
:MobileBuildPlatforms
MobileTestPlatforms
Mobile platform settings are read from
mobile_config.json
file in.yamato
folder.Another difference between editor/standalone and mobile tests recipes is:
BaseRecipe
MobileBaseRecipe
which inheritsBaseRecipe
. That's because I need to add support for bothmono
andil2cpp
backends for Android. I did some customization for Android about how to create jobs for it. The details can be seen inMobileBaseRecipe.cs
.The major differences between functional tests recipes and performance tests recipes are in UtrCommand.
WithCategory("!Performance")
and--enable-code-coverage
..WithCategory("Performance")
andWithPerformanceDataReporting
.Other than that, functional tests recipes and performance tests recipes are very similar.
Please note that: In the old Build & Run jobs on Desktop OS platforms, upm-ci package test can be replaced by Wrench validation jobs. That's why I didn't add it in the recipes. For example, the old PVS tests have been replaced by
upm-pvp test
in Wrench validation jobs. Wrench validation jobs also run tests inside the package, so there is no need to run package tests in other jobs.Testing status & QA
Please describe the testing already done by you and what testing you request/recommend QA to execute. If you used or created any testing project please link them here too for QA.
Overall Product Risks
Please rate the potential complexity and halo effect from low to high for the reviewers. Note down potential risks to specific Editor branches if any.
Comments to reviewers
Please describe any additional information such as what to focus on, or historical info for the reviewers.
Checklist
Before review:
Changed
,Fixed
,Added
sections.Area_CanDoX
,Area_CanDoX_EvenIfYIsTheCase
,Area_WhenIDoX_AndYHappens_ThisIsTheResult
.During merge:
NEW: ___
.FIX: ___
.DOCS: ___
.CHANGE: ___
.RELEASE: 1.1.0-preview.3
.After merge: