Skip to content

Conversation

@thomas-tu
Copy link
Collaborator

Purpose of this PR

Some tests were disable last July on trunk as they were failing. This PR brings the changes back to the package repository.

Links

Jira: https://jira.unity3d.com/browse/PBLD-248

Comments to Reviewers

[List known issues, planned work, provide any extra context for your code.]

@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:

🎫 Ticket compliance analysis ✅

PBLD-248 - Fully compliant

Compliant requirements:

  • The PR implements a fix by disabling the failing tests specifically on the macOS Arm64 environment, which should stabilize the pipeline.
⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪

The changes are straightforward and repetitive, adding a platform check to skip specific tests.
🏅 Score: 95

The PR correctly implements the requested temporary fix (disabling tests) with appropriate platform guards.
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Verify Test Class Location

The ticket refers to IntVectorTests as the failing class, but the modifications are in VertexTests.cs. While the method names match the ticket description exactly, please verify that this is the correct file/class and that the ticket description wasn't referring to a different test suite with similar names.

public static void TestHashCollisions_IVEC3()
{
  • 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
Use ProcessArchitecture to target native Arm64

Use ProcessArchitecture instead of OSArchitecture. OSArchitecture returns Arm64 even
when running the Intel Editor on Apple Silicon (via Rosetta), which might not
exhibit the issue. ProcessArchitecture correctly targets the native Arm64 process.
Apply this change to all similar checks in the file.

Tests/Runtime/Type/VertexTests.cs [61-64]

 #if UNITY_EDITOR_OSX
-        if (System.Runtime.InteropServices.RuntimeInformation.OSArchitecture == System.Runtime.InteropServices.Architecture.Arm64)
+        if (System.Runtime.InteropServices.RuntimeInformation.ProcessArchitecture == System.Runtime.InteropServices.Architecture.Arm64)
             Assert.Ignore("Fails on macOS13 Arm64 https://jira.unity3d.com/browse/UUM-111993");
 #endif
Suggestion importance[1-10]: 7

__

Why: OSArchitecture returns Arm64 even when running an x64 process via Rosetta on macOS, potentially causing tests to be skipped unnecessarily if the issue is specific to the native Arm64 instruction set. ProcessArchitecture ensures the check targets the actual running process architecture.

Medium
  • More suggestions

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

@varinotmUnity
Copy link
Contributor

Is this an issue with one of our commit, or a missing update on our side since the editor upgrade?

Copy link
Contributor

@varinotmUnity varinotmUnity left a comment

Choose a reason for hiding this comment

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

lgtm
I'm guessing we can't simply do [UnityPlatform(exclude = new[] { RuntimePlatform.OSXEditor })] for arm64 only

@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     #638   +/-   ##
=======================================
  Coverage   35.57%   35.57%           
=======================================
  Files         277      277           
  Lines       34892    34892           
=======================================
  Hits        12413    12413           
  Misses      22479    22479           
Flag Coverage Δ
mac_trunk 35.34% <ø> (ø)
win_trunk 35.23% <ø> (ø)

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

🚀 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.

@thomas-tu
Copy link
Collaborator Author

lgtm I'm guessing we can't simply do [UnityPlatform(exclude = new[] { RuntimePlatform.OSXEditor })] for arm64 only

No indeed :(

@thomas-tu thomas-tu mentioned this pull request Nov 20, 2025
@thomas-tu thomas-tu merged commit 284a0b9 into master Nov 20, 2025
11 checks passed
@thomas-tu thomas-tu deleted the test/disable-failing-tests-on-arm64 branch November 20, 2025 19:30
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.

3 participants