CHANGE: Migrate ci to recipe engine#2226
Conversation
windxu88
changed the title
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:
|
stefanunity
left a comment
stefanunity
left a comment
There was a problem hiding this comment.
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.
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.
| .WithDependencies(allMobileFunctionalTests) | ||
| .WithDependencies(allTvOSFunctionalBuildJobs), | ||
|
|
||
| JobBuilder.Create("All Functional Tests") |
There was a problem hiding this comment.
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
jfreire-unity
left a comment
jfreire-unity
left a comment
There was a problem hiding this comment.
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! 🚀 🙏
|
|
||
| namespace InputSystem.Cookbook.Recipes; | ||
|
|
||
| public abstract class InputBaseRecipe: RecipeBase |
There was a problem hiding this comment.
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.
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.
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.
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.
I don't see a WithPlatform here. Is it expected?
There was a problem hiding this comment.
"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.
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.
Have added some comments
| @@ -1,2 +1,2 @@ | |||
| cd $(dirname "$0")/../../ | |||
| dotnet run --project Tools\CI\InputSystem.Cookbook.csproj No newline at end of file | |||
| dotnet run --project Tools/CI/InputSystem.Cookbook.csproj No newline at end of file | |||
There was a problem hiding this comment.
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.
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
jfreire-unity
left a comment
jfreire-unity
left a comment
There was a problem hiding this comment.
Thanks for doing this! Approving to not leave this lingering longer. No blockers for me.
4 participants
Description
This PR migrates CI to use Recipe Engine.
In Input CI, tests fall into two categories:
functional testsandperformance tests.For each category, tests run on the following Runtime platforms:
Windows,UbuntuandMacOSTvOS,iOSandAndroid. For Android, we cover bothmonoandil2cppbackends.In the migration, I created four recipes for both
FunctionalandPerformancetests.For functional tests, they are:
EditorFunctionalTestsStandaloneFunctionalTestsStandaloneIl2CppFunctionalTestsMobileFunctionalTestsFor performance tests, there are four equivalents:
EditorPerformanceTestsStandalonePerformanceTestsStandaloneIl2CppPerformanceTestsMobilePerformanceTestsFor editor and standalone tests recipes, the tests will run on
WrenchPackage.DefaultEditorPlatformswhich are Windows, Mac and Ubuntu. That's what we want.For mobile tests recipe, I added the following platforms in
InputSystemSettings.cs:MobileBuildPlatformsMobileTestPlatformsMobile platform settings are read from
mobile_config.jsonfile in.yamatofolder.Another difference between editor/standalone and mobile tests recipes is:
BaseRecipeMobileBaseRecipewhich inheritsBaseRecipe. That's because I need to add support for bothmonoandil2cppbackends 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 testin 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,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.After merge: