Skip to content

Update Wrench to 2.3.2 for V2#1111

Merged
windxu88 merged 3 commits intorelease/2.10from
update_ci_for_cinemachine_v2
Mar 16, 2026
Merged

Update Wrench to 2.3.2 for V2#1111
windxu88 merged 3 commits intorelease/2.10from
update_ci_for_cinemachine_v2

Conversation

@windxu88
Copy link
Copy Markdown
Collaborator

@windxu88 windxu88 commented Mar 16, 2026

[Delete any line or section that does not apply]

Purpose of this PR

  • Updated Wrench to 2.3.2 for V2
  • Enabled code coverage
  • Added clean console tests

Testing status

[Explanation of what’s tested, how tested and existing or new automation tests. Can include manual testing by self and/or QA. Specify test plans. Rarely acceptable to have no testing.]

  • Added an automated test
  • Passed all automated tests
  • Manually tested

Documentation status

[Overview of how documentation is affected by this change. If there is no effect on documentation, explain why. Otherwise, state which sections are changed and why.]

  • Updated CHANGELOG
  • Updated README (if applicable)
  • Commented all public classes, properties, and methods
  • Updated user documentation

Technical risk

[Overall product level assessment of risk of change. Need technical risk & halo effect.]

Comments to reviewers

[Info per person for what to focus on, or historical info to understand who have previously reviewed and coverage. Help them get context.]

Package version

[Justification for updating either the patch, minor, or major version according to the semantic versioning rules]

  • Updated package version

@windxu88 windxu88 changed the base branch from main to release/2.10 March 16, 2026 16:42
@windxu88 windxu88 requested a review from glabute as a code owner March 16, 2026 16:42
@u-pr
Copy link
Copy Markdown
Contributor

u-pr bot commented Mar 16, 2026

May require changes
Several high-priority issues were identified, including broken CI triggers due to invalid regex, path casing errors that will break scripts on Linux/macOS, incorrect assembly filters for code coverage, and case-sensitivity issues in the .gitignore. A few minor typos and coding style suggestions were also noted.

🤖 Helpful? 👍/👎

@u-pr
Copy link
Copy Markdown
Contributor

u-pr bot commented Mar 16, 2026

Suggestions for Tools/Cinemachine-Recipes/Recipes/Triggers.cs:

high
There is an unbalanced closing parenthesis ) at the end of this regex string. This generates an invalid regex pattern release[/]\d+[.]\d+) in .yamato/triggers.yml, which will cause regex parsing errors and fail to trigger CI jobs for release branches.

@u-pr
Copy link
Copy Markdown
Contributor

u-pr bot commented Mar 16, 2026

Suggestions for Tools/Cinemachine-Recipes/regenerate.sh:

high
The directory name has a casing typo (CineMachine-Recipes instead of Cinemachine-Recipes). Because Linux and macOS file systems are case-sensitive by default, running this script will fail with a "project not found" error on those platforms.

@u-pr
Copy link
Copy Markdown
Contributor

u-pr bot commented Mar 16, 2026

Suggestions for Tools/Cinemachine-Recipes/regenerate.bat:

low
The directory name has the same casing typo as in regenerate.sh. While Windows is typically case-insensitive, it's best to fix this to keep paths consistent.

@u-pr
Copy link
Copy Markdown
Contributor

u-pr bot commented Mar 16, 2026

Suggestions for .gitignore:

high
The directory name uses a lowercase p, but the actual directory in the repository is Projects (with a capital P). Since Git is case-sensitive by default on Linux, these patterns will fail to ignore the Library, Temp, and other generated directories inside Projects/ when cloned on a case-sensitive file system.

@u-pr
Copy link
Copy Markdown
Contributor

u-pr bot commented Mar 16, 2026

Suggestions for Tools/Cinemachine-Recipes/Recipes/CodeCoverage.cs:

high
The assemblyFilters parameter is configured to +Unity.Cinemachine*. However, in the Cinemachine 2.x codebase, the assemblies are named Cinemachine and com.unity.cinemachine.editor (as confirmed by UnitTestInitialization.asmdef).

Using +Unity.Cinemachine* will fail to match the actual assemblies and result in 0% code coverage. This filter might have been accidentally backported from Cinemachine 3.x.

@windxu88 windxu88 requested review from kroatoa and sim-bz and removed request for kroatoa March 16, 2026 18:43
@windxu88
Copy link
Copy Markdown
Collaborator Author

/review

@u-pr
Copy link
Copy Markdown
Contributor

u-pr bot commented Mar 16, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

The PR is mostly CI/RecipeEngine/Wrench migration with many generated YAML changes, so reviewing correctness requires validating triggers, platform/editor matrices, and coverage plumbing across multiple jobs.

🏅 Score: 77

The migration looks directionally correct, but there are a few high-risk correctness gaps (branch/trigger filtering, unsafe .First() usage, and potential dictionary key/quoting issues) that could break CI or reduce coverage reliability.

🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Crash

The code assumes at least one Unity editor exists (.UnityEditors.First()); if the list is empty or misconfigured for the package, recipe generation will throw and break CI generation.

private static readonly Platform
    Platform = Settings.Wrench.Packages[PackageName].UnityEditors.First().AllEditorPlatforms[EditorPlatformType.MacOs13];
Key Lookup Risk

settings.Wrench.EditorVersionToBranches[unityEditor.Version.ToString()] can throw if the mapping is missing/format-changed for a given editor version; consider a safer lookup/fallback to avoid generation failures.

var allSupportedEditors =
    settings.Wrench.Packages[CinemachineSettings.packageName].UnityEditors;

List<IJobBuilder> builders = new();
foreach (var unityEditor in allSupportedEditors)
{
    foreach (var platform in unityEditor.EditorPlatforms)
    {
        var jobName = GetJobName(unityEditor, platform.System);
        IJobBuilder job = JobBuilder.Create(jobName)
            .WithDescription(
                $"Clean console tests for {settings.Wrench.Packages[CinemachineSettings.packageName].DisplayName} on {platform.System}")
            .WithPlatform(platform)
            .WithCommands(c => c
                .AddBrick("git@github.cds.internal.unity3d.com:wind-xu/clean_console_test_brick.git@v0.3.7",
                    ("EDITOR_VERSION", settings.Wrench.EditorVersionToBranches[unityEditor.Version.ToString()]),
                    ("CLEAN_CONSOLE_TEST_FOR", "package"),
                    ("PACKAGE_PATH", CinemachineSettings.packageName),
                    ("EXEMPTION_FILE_DIR", ".yamato"),
                    ("WARNINGS_AS_ERRORS", false))
Style/Immutability

The settings field is not readonly and has non-standard casing relative to other recipes; making it private readonly improves clarity and avoids accidental reassignment.

CinemachineSettings settings = new();

protected override ISet<Job> LoadJobs()
Coverage Args Fragility

The --coverage-options/--coverage-upload-options strings are heavily quoted/escaped and include platform-specific env var substitutions; validate the final command line on all OSes to ensure quoting doesn’t break argument parsing and that pathReplacePatterns/sourcePaths are correct.

.Add(UtrCommand.Run(platform.System, b => b
    .WithTestProject($"{settings.ProjectsDir}/{project}")
    .WithEditor(".Editor")
    .WithRerun(1, true)
    .WithArtifacts("artifacts")
    .WithSuite(UtrTestSuiteType.Editor)
    .WithExtraArgs("--suite=PlayMode")
    .WithExtraArgs("--enable-code-coverage", 
    "--coverage-options=\"generateAdditionalMetrics;generateHtmlReport;" + 
    $"assemblyFilters:+Cinemachine,+com.unity.cinemachine.editor;pathReplacePatterns:@*,,**/PackageCache/,;sourcePaths:{yamatoSourceDir}/Packages;\"",
    $"--coverage-results-path={yamatoSourceDir}/upm-ci~/CodeCoverage",
    $"--coverage-upload-options=\"reportsDir:upm-ci~/CodeCoverage;name:cinemachine_{platform.System.ToString()}_{version}_project;flags:cinemachine_{platform.System.ToString()}_{version}_project\""))))
Trigger Filtering

Branch filtering moved to branchName plus a regex; verify the regex is valid and that it matches the intended branches (also check that branchName matches the actual default branch used in this repo for V2).

private const string branchName = "release/2.10";

protected override ISet<Job> LoadJobs()
    => Combine.Collections(GetTriggers()).SelectJobs();

private ISet<IJobBuilder> GetTriggers()
{
    HashSet<IJobBuilder> builders = new();
    var validationTests = config.Wrench.WrenchJobs[packageName][JobTypes.Validation];
    var projectTests = new ProjectTest().AsDependencies();
    var cleanConsoleTests = new CleanConsoleTests().AsDependencies();

    builders.Add(JobBuilder.Create($"Nightly Trigger")
        .WithDescription($"Nightly check on {branchName}")
        .WithDependencies(projectTests)
        .WithDependencies(validationTests)
        .WithDependencies(cleanConsoleTests)
        .WithScheduleTrigger(Schedule.RunDaily(branchName))
    );

    builders.Add(JobBuilder.Create($"All Trigger")
        .WithDependencies(projectTests)
        .WithDependencies(validationTests)
        .WithBranchesTrigger(b => b.Only(branchName, "release[/]\\\\d+[.]\\\\d+)"))
        .WithDescription("All tests defined in recipes. Run in changes to main and release branches.")
    );
PR Subset Selection

PR job selection relies on substring checks (JobId.Contains("Windows") and Contains("win")); verify casing and naming conventions match generated job IDs, otherwise PR triggers may run too little or nothing at all.

var prProjectTests = projectTests.Where(job => job.JobId.Contains("Windows"));
var prValidationTests = config.Wrench.WrenchJobs[packageName][JobTypes.Validation].Where(job => job.JobId.Contains("win"));

builders.Add(JobBuilder.Create("Pull Request Tests Trigger")
    .WithDependencies(prProjectTests)
    .WithDependencies(prValidationTests)
    .WithPullRequestTrigger(pr => pr.ExcludeDraft(),
        true, CancelLeftoverJobs.Always)
    .WithDescription("Tests to run on PRs.")
);
Coverage Config

Coverage is enabled and assembly allow-list patterns are added; confirm the assembly names/patterns match what UTR/code-coverage emits (and that editor assembly name casing is correct), otherwise coverage can silently upload empty/partial reports.

    Wrench.Packages[packageName].CoverageCommands.Enabled = true;
    Wrench.Packages[packageName].CoverageCommands.AssemblyAllowList.Add("^Cinemachine$");
    Wrench.Packages[packageName].CoverageCommands.AssemblyAllowList.Add("^com.unity.cinemachine.editor$");

    Wrench.PvpProfilesToCheck = PvPprofilesToCheck;
}
Shell Robustness

The cd command isn’t quoted; paths with spaces can break recipe regeneration locally/CI. Quoting the command substitution and paths would make the script more robust.

cd $(dirname "$0")/../../
dotnet run --project Tools/Cinemachine-Recipes/Cinemachine.Cookbook.csproj
Windows Path Robustness

The cd %~dp0../../ is unquoted and doesn’t use cd /d; this can fail when the repo is on a different drive or contains spaces.

cd %~dp0../../
dotnet run --project Tools/Cinemachine-Recipes/Cinemachine.Cookbook.csproj
  • Update review

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr

@codecov-github-com
Copy link
Copy Markdown

codecov-github-com bot commented Mar 16, 2026

Codecov Report

All modified and coverable lines are covered by tests ✅

Impacted file tree graph

@@               Coverage Diff               @@
##             release/2.10    #1111   +/-   ##
===============================================
  Coverage                ?   25.17%           
===============================================
  Files                   ?      137           
  Lines                   ?    20216           
  Branches                ?        0           
===============================================
  Hits                    ?     5089           
  Misses                  ?    15127           
  Partials                ?        0           
Flag Coverage Δ
cinemachine_Windows_2022.3 15.58% <ø> (?)
cinemachine_Windows_2022.3_project 25.12% <ø> (?)
cinemachine_Windows_6000.0 15.58% <ø> (?)
cinemachine_Windows_6000.0_project 25.07% <ø> (?)
cinemachine_Windows_6000.3 15.58% <ø> (?)
cinemachine_Windows_6000.3_project 25.07% <ø> (?)
cinemachine_Windows_6000.4 15.58% <ø> (?)
cinemachine_Windows_6000.4_project 25.09% <ø> (?)
cinemachine_Windows_6000.5 15.58% <ø> (?)
cinemachine_Windows_6000.5_project 25.07% <ø> (?)
cinemachine_Windows_6000.6 15.58% <ø> (?)
cinemachine_Windows_6000.6_project 25.09% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

@windxu88 windxu88 merged commit fb87586 into release/2.10 Mar 16, 2026
31 of 32 checks passed
@windxu88 windxu88 deleted the update_ci_for_cinemachine_v2 branch March 16, 2026 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants