Skip to content

Comments

CHANGE: Expose internals to the input tests package #2256

Closed
AswinRajGopal wants to merge 1 commit intodevelopfrom
input-tests-package-expose-internals
Closed

CHANGE: Expose internals to the input tests package #2256
AswinRajGopal wants to merge 1 commit intodevelopfrom
input-tests-package-expose-internals

Conversation

@AswinRajGopal
Copy link
Collaborator

@AswinRajGopal AswinRajGopal commented Oct 8, 2025

Description

This change has dependency for this test package PR: https://github.cds.internal.unity3d.com/unity/unity/pull/78036

Testing status & QA

Overall Product Risks

  • Complexity:
  • Halo Effect:

Comments to reviewers

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
    • Jira port for the next release set as "Resolved".
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

After merge:

  • Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

@u-pr
Copy link
Contributor

u-pr bot commented Oct 8, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

The PR consists of only three new lines adding `InternalsVisibleTo` attributes, which is a straightforward and easy-to-review change.
🏅 Score: 100

The change is correct, minimal, and directly addresses the stated goal of exposing internal APIs to test assemblies without introducing any risks.
🧪 No relevant tests
🔒 No security concerns identified
⚡ No major issues detected

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

@u-pr
Copy link
Contributor

u-pr bot commented Oct 8, 2025

PR Code Suggestions ✨

No code suggestions found for the PR.

@codecov-fd.fxlcf.dpdns.org
Copy link

codecov-fd.fxlcf.dpdns.org bot commented Oct 8, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

@@             Coverage Diff             @@
##           develop    #2256      +/-   ##
===========================================
+ Coverage    76.70%   76.72%   +0.01%     
===========================================
  Files          465      465              
  Lines        87919    87944      +25     
===========================================
+ Hits         67442    67477      +35     
+ Misses       20477    20467      -10
Flag Coverage Δ
inputsystem_MacOS_2021.3 5.91% <ø> (-0.01%) ⬇️
inputsystem_MacOS_2022.3 5.37% <ø> (-0.01%) ⬇️
inputsystem_MacOS_2022.3_project 74.60% <ø> (+0.01%) ⬆️
inputsystem_MacOS_6000.0 5.18% <ø> (-0.01%) ⬇️
inputsystem_MacOS_6000.0_project 76.52% <ø> (+0.01%) ⬆️
inputsystem_MacOS_6000.2 5.18% <ø> (-0.01%) ⬇️
inputsystem_MacOS_6000.2_project 76.53% <ø> (+0.02%) ⬆️
inputsystem_MacOS_6000.3 5.19% <ø> (-0.01%) ⬇️
inputsystem_MacOS_6000.3_project 76.52% <ø> (+0.01%) ⬆️
inputsystem_MacOS_6000.4 5.19% <ø> (-0.01%) ⬇️
inputsystem_MacOS_6000.4_project 76.52% <ø> (+0.01%) ⬆️
inputsystem_Ubuntu_2021.3 5.91% <ø> (-0.01%) ⬇️
inputsystem_Ubuntu_2022.3 5.38% <ø> (-0.01%) ⬇️
inputsystem_Ubuntu_2022.3_project 74.41% <ø> (+0.01%) ⬆️
inputsystem_Ubuntu_6000.0 5.19% <ø> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.0_project 76.33% <ø> (+0.01%) ⬆️
inputsystem_Ubuntu_6000.2 5.19% <ø> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.2_project 76.33% <ø> (+0.01%) ⬆️
inputsystem_Ubuntu_6000.3 5.19% <ø> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.3_project 76.33% <ø> (+0.01%) ⬆️
inputsystem_Ubuntu_6000.4 5.19% <ø> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.4_project 76.33% <ø> (+0.01%) ⬆️
inputsystem_Windows_2021.3 5.91% <ø> (-0.01%) ⬇️
inputsystem_Windows_2022.3 5.37% <ø> (-0.01%) ⬇️
inputsystem_Windows_2022.3_project 74.74% <ø> (+0.01%) ⬆️
inputsystem_Windows_6000.0 5.19% <ø> (-0.01%) ⬇️
inputsystem_Windows_6000.0_project 76.66% <ø> (+0.01%) ⬆️
inputsystem_Windows_6000.2 5.19% <ø> (-0.01%) ⬇️
inputsystem_Windows_6000.2_project 76.66% <ø> (+0.01%) ⬆️
inputsystem_Windows_6000.3 5.19% <ø> (-0.01%) ⬇️
inputsystem_Windows_6000.3_project 76.66% <ø> (+0.01%) ⬆️
inputsystem_Windows_6000.4 5.19% <ø> (-0.01%) ⬇️
inputsystem_Windows_6000.4_project 76.66% <ø> (+0.01%) ⬆️

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

see 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AswinRajGopal AswinRajGopal requested a review from LeoUnity October 8, 2025 16:15
@ekcoh
Copy link
Collaborator

ekcoh commented Oct 9, 2025

Why is this internal visibility needed? What dependencies exist driving such a change?

@LeoUnity
Copy link
Collaborator

LeoUnity commented Oct 9, 2025

Why is this internal visibility needed? What dependencies exist driving such a change?

This is related to the task Ashwin is working on where he is moving tests in trunk, from a project to a package.

There are 2 types that those tests access that needs access to internals:

@LeoUnity
Copy link
Collaborator

LeoUnity commented Oct 9, 2025

That being said, DualShock4HIDInputReport should be accessible because it used from Tests/EditModeAndPlayModeTests/InputBackend/Assets/Utilities/InputUtils.cs and that lives in the Tests/EditModeAndPlayModeTests/InputBackend/Assets/Utilities/Utilities.asmdef which is named Unity.InputSystem.Tests, which already has access to internals...
So it shouldn't be needed...

@ekcoh
Copy link
Collaborator

ekcoh commented Oct 10, 2025

Why is this internal visibility needed? What dependencies exist driving such a change?

This is related to the task Ashwin is working on where he is moving tests in trunk, from a project to a package.

There are 2 types that those tests access that needs access to internals:

I am just worried this is adding to the pile of technical guilt that needs to be reverted. @LeoUnity @AswinRajGopal here is my perspective on this.

DualShock4HIDFormat e.g:
public static FourCC Format = new FourCC('D', '4', 'V', 'S'); // DualShock4 Virtual State
should not be in Input System at all, this is just wrong to begin with and something we hope to remedy or make obsolete with initial phases of our module work that has not been executed yet, it will go away eventually, and is actually defined in native so Input System is just duplicating the FourCC code instead of referencing Input module where it actually should be defined. I would recommend just defining this as a constant for now to work around this and add another ticket into Milestone A to remove this from these tests as that is addressed. E.g. this would basically be something like this in the future in Input System: public static FourCC Format = UnityEngineInternal.Input.LayoutConstants.DualShock4. (doesn't exist now)

The EditorPlayerSettingHelpers class is also kind of in the wrong place at the moment. Its code to set/get a string setting in Unity has really has nothing to do with the Input System. It's a utility for Unity Player Settings to see what the active input backend setting is. It is also not much code if you strip out the unnecessary #ifdefs from there so I would vote to rather replicate it for this use case. Or add it as an internal class to module and give access to this test from there.

I am not sure there was additional thing I can help with, feel free to DM in that case or reply here.

@ekcoh ekcoh self-requested a review October 10, 2025 08:36
ekcoh
ekcoh requested changes Oct 10, 2025
View reviewed changes
Copy link
Collaborator

@ekcoh ekcoh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I am marking this as request changes since I think we should avoid adding these kind of dependencies unless there is no other way. I am more than happy to help guiding towards alternatives.

@ekcoh ekcoh changed the title Expose internals to the input tests package Change: Expose internals to the input tests package Oct 10, 2025
@ekcoh ekcoh changed the title Change: Expose internals to the input tests package CHANGE: Expose internals to the input tests package Oct 10, 2025
@ekcoh
Copy link
Collaborator

ekcoh commented Oct 21, 2025

@AswinRajGopal Please update this PR with latest develop to get #2260, I do not dare updating it since it has conflicts.

@AswinRajGopal
Copy link
Collaborator Author

AswinRajGopal commented Oct 23, 2025

Agreed, dealt this for now by creating bridge asmdefs which already had exposure to internals. Closing this PR. Thanks @ekcoh @LeoUnity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ekcoh ekcoh ekcoh requested changes

@LeoUnity LeoUnity Awaiting requested review from LeoUnity

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@AswinRajGopal @ekcoh @LeoUnity