Skip to content

Conversation

@thomas-tu
Copy link
Collaborator

@thomas-tu thomas-tu commented Nov 20, 2025

DO NOT FORGET TO INCLUDE A CHANGELOG ENTRY

Purpose of this PR

This PR makes a couple of updates:

  • Run tests on MacOS Arm64 to ensure we capture potential failures with this architecture on this repository.
  • Pass --fail-on-assert to ensure we don't miss any assert.
  • Update Wrench to 1.3.0.

Links

Jira:

Comments to Reviewers

This PR is depends on #638.

@codecov-github-com
Copy link

codecov-github-com bot commented Nov 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

@@            Coverage Diff             @@
##           master     #639      +/-   ##
==========================================
- Coverage   35.57%   35.56%   -0.01%     
==========================================
  Files         277      277              
  Lines       34892    34892              
==========================================
- Hits        12413    12410       -3     
- Misses      22479    22482       +3     
Flag Coverage Δ
mac_trunk 35.34% <ø> (ø)
probuilder_MacOS_6000.0 35.32% <ø> (-0.03%) ⬇️
probuilder_MacOS_6000.2 35.32% <ø> (-0.03%) ⬇️
probuilder_MacOS_6000.3 35.32% <ø> (-0.03%) ⬇️
probuilder_MacOS_6000.4 35.32% <ø> (-0.04%) ⬇️
probuilder_MacOS_6000.5 35.32% <ø> (-0.03%) ⬇️
probuilder_Ubuntu_6000.0 35.20% <ø> (ø)
probuilder_Ubuntu_6000.2 35.20% <ø> (ø)
probuilder_Ubuntu_6000.3 35.21% <ø> (ø)
probuilder_Ubuntu_6000.4 35.21% <ø> (ø)
probuilder_Ubuntu_6000.5 35.21% <ø> (ø)
probuilder_Windows_6000.0 35.24% <ø> (ø)
probuilder_Windows_6000.2 35.24% <ø> (ø)
probuilder_Windows_6000.3 35.24% <ø> (ø)
probuilder_Windows_6000.4 35.24% <ø> (ø)
probuilder_Windows_6000.5 35.24% <ø> (ø)
win_trunk 35.23% <ø> (ø)

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

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@u-pr-agent
Copy link

u-pr-agent bot commented Nov 20, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

The PR contains small configuration changes in a single file, which are straightforward to verify.
🏅 Score: 95

The code is clean and achieves the stated goals, with only a minor consistency issue regarding a hardcoded string.
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Use Constant

The package name "com.unity.probuilder" is hardcoded here. Consider using the existing ProBuilderPackageName constant (used in lines 22 and 41) for consistency and to avoid magic strings.

Wrench.Packages["com.unity.probuilder"].EditorPlatforms[SystemType.MacOS] = new Platform(new Agent("package-ci/macos-13-arm64:v4", FlavorType.MacDefault, defaultMacPlatform.Agent.Resource, "M1"), defaultMacPlatform.System);
  • Update review

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

@u-pr-agent
Copy link

u-pr-agent bot commented Nov 20, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Replace hardcoded string with defined constant

Use the existing ProBuilderPackageName constant instead of the hardcoded string
literal "com.unity.probuilder". This ensures consistency with the rest of the
constructor and prevents runtime errors if the package name constant is updated in
the future.

Tools/CI/Settings/ProBuilderSettings.cs [45]

-Wrench.Packages["com.unity.probuilder"].EditorPlatforms[SystemType.MacOS] = new Platform(new Agent("package-ci/macos-13-arm64:v4", FlavorType.MacDefault, defaultMacPlatform.Agent.Resource, "M1"), defaultMacPlatform.System);
+Wrench.Packages[ProBuilderPackageName].EditorPlatforms[SystemType.MacOS] = new Platform(new Agent("package-ci/macos-13-arm64:v4", FlavorType.MacDefault, defaultMacPlatform.Agent.Resource, "M1"), defaultMacPlatform.System);
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the package name string is hardcoded while a constant ProBuilderPackageName is already defined and used nearby. Using the constant improves consistency and maintainability.

Medium
  • More suggestions

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

@thomas-tu thomas-tu force-pushed the ci/update-wrench-and-run-on-macos-arm64 branch from 1d140cf to c5b7f55 Compare November 20, 2025 19:32
@windxu88
Copy link

windxu88 commented Nov 20, 2025

Many jobs are failing:(

Edit: Probably because Wrench 1.3.0 has some bug. Like I said, it is not very stable. We can try an older version to see.

@thomas-tu
Copy link
Collaborator Author

@windxu88 I've looked at the errors and it's expected. They will be fixed with #637. Sorry, I forgot to mention it in the description.

@thomas-tu thomas-tu force-pushed the ci/update-wrench-and-run-on-macos-arm64 branch from c5b7f55 to 7b2ef9e Compare November 24, 2025 20:38
Copy link

@windxu88 windxu88 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@thomas-tu thomas-tu merged commit 7114bb0 into master Nov 24, 2025
47 checks passed
@thomas-tu thomas-tu deleted the ci/update-wrench-and-run-on-macos-arm64 branch November 24, 2025 21:24
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.

4 participants