Skip to content

Conversation

@mdaigle
Copy link
Contributor

@mdaigle mdaigle commented Mar 24, 2025

Internal types are not accessible outside of their home project. However, they still need to be tested. This PR exposes internal types to the ManualTests and FunctionalTests projects so that they can be used directly instead of via reflection.

This PR does not update any tests that currently use reflection. Those can be addressed separately as tests are updated or as code health PRs.

@mdaigle mdaigle marked this pull request as ready for review March 24, 2025 16:47
@mdaigle mdaigle requested a review from a team March 24, 2025 16:48
@mdaigle mdaigle added the Area\Tests Issues that are targeted to tests or test projects label Mar 24, 2025
@mdaigle mdaigle changed the title Expose internal types to tests Test | Expose internal types to tests Mar 24, 2025
<TargetFrameworkMonikerAssemblyAttributesPath>$([System.IO.Path]::Combine('$(IntermediateOutputPath)','$(TargetFramework)','$(TargetFrameworkMoniker).AssemblyAttributes$(DefaultLanguageSourceExtension)'))</TargetFrameworkMonikerAssemblyAttributesPath>
</PropertyGroup>
<ItemGroup>
<InternalsVisibleTo Include="ManualTests" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This syntax is only available in SDK style projects that use .NET 5+. Our only option for netfx was direct assembly tags.

<TargetFrameworkMonikerAssemblyAttributesPath>$([System.IO.Path]::Combine('$(IntermediateOutputPath)','$(TargetFramework)','$(TargetFrameworkMoniker).AssemblyAttributes$(DefaultLanguageSourceExtension)'))</TargetFrameworkMonikerAssemblyAttributesPath>
</PropertyGroup>
<ItemGroup>
<InternalsVisibleTo Include="ManualTests" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I love it, but do we need to restrict via strong names and public keys?

https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-runtime-compilerservices-internalsvisibletoattribute

As-is, I think this would let anyone pair their assembly named "ManualTests" with our MDS package, and easily access internals. Maybe that isn't a concern though, since they can bypass the whole visibility system via reflection anyway. Thoughts?

Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

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

Love the idea - we've needed it for a long time - but need to see it pass before I can approve :)

@codecov
Copy link

codecov bot commented Mar 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.72%. Comparing base (adce139) to head (7dde277).
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3239      +/-   ##
==========================================
- Coverage   72.76%   72.72%   -0.05%     
==========================================
  Files         303      300       -3     
  Lines       59527    59616      +89     
==========================================
+ Hits        43317    43356      +39     
- Misses      16210    16260      +50     
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 75.18% <ø> (+<0.01%) ⬆️
netfx 71.37% <ø> (-0.05%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

@paulmedynski paulmedynski self-assigned this Mar 25, 2025
@mdaigle mdaigle linked an issue Mar 25, 2025 that may be closed by this pull request
@mdaigle mdaigle changed the title Test | Expose internal types to tests Test | Expose internal types to ManualTests Mar 25, 2025
Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

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

Let's gooooooooooooooooooooooooooooooooooooo 🚢 🚢 🚢 🚀

@mdaigle mdaigle merged commit 41b9345 into main Mar 27, 2025
252 checks passed
@mdaigle mdaigle deleted the dev/mdaigle/expose-internals-to-tests branch March 27, 2025 17:25
@mdaigle mdaigle added this to the 7.0-preview1 milestone Mar 27, 2025
mdaigle added a commit that referenced this pull request Apr 10, 2025
cheenamalhotra pushed a commit that referenced this pull request Apr 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area\Tests Issues that are targeted to tests or test projects

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable InternalsVisibleTo Test Projects

4 participants