Introduce non-shipped System.Runtime.Internal assembly #61754
Conversation
Introduce non-shipped System.Runtime.Internal assembly for internal test usage only and remove <ReferenceSystemPrivateCoreLib> support from our test system. Any internal APIs (primarily in the hosting space) that are needed for testing that we don't want to expose publically should be exposed via System.Runtime.Internal for our testing infrastructure to reference.
jkoritzinsky
requested review from
lambdageek,
jkotas,
ericstj and
AaronRobinsonMSFT
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure Issue DetailsIntroduce non-shipped System.Runtime.Internal assembly for internal test usage only and remove support from our test system. Any internal APIs (primarily in the hosting space) that are needed for testing that we don't want to expose publicly should be exposed via System.Runtime.Internal for our testing infrastructure to reference. This allows us to simplify our testing infrastructure to not have to handle tests that reference only CoreLib and to not have to build CoreLib in our test build job.
|
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Co-authored-by: Aaron Robinson <[email protected]>
|
@AaronRobinsonMSFT - I think that "never designed to be shipped" is impossible to define. The newly introduced S.R.I just adds support for stuff that tests need but the product doesn't have a reason or desire to ship [yet, potentially]. |
… to make it more difficult to accidentally ship.
…time into system.runtime.internal
jkoritzinsky
force-pushed
the
system.runtime.internal
branch
from
b0d5c9e
to
1cd5be6
Compare
|
We already have validation that we don't ship files that we don't expect in the Shared Framework SDK, so we don't need to worry about it implicitly being included there. Out-of-box packaging is an explicit opt-in, but just in case I've added an explicit MSBuild error if someone tries to pack the assembly into a NuGet package on its own. |
This works for me. I much prefer having that. |
I suppose. The point though is we need "expected to ship" or "not expected to ship" or at least the concept. In this case a verification step for the SDK handles the "expected to ship" verification so that means we intentionally say "ship it" somewhere and that was my general concern. I'm happy as long as we have a validation in one direction. |
|
My personal preference would be to test the internal interfaces using reflection instead of introducing the internal contracts. IIRC, I had same feedback when some of these were introduced. It also matches how we are generally testing the internal details under libraries - we not build internal contracts for those. |
@jkotas I recall that. I think the biggest annoyance there is fiddling with the Reflection API. In this case, we would need to create a new instance of types and set fields. I guess this could be performed with |
If you count number of lines, I think it would be less than the annoyance of introducing the internal contracts. ICastable is annoying (you would have to use a bit of reflection.emit for it), but I hope that it will go away once we get the new source generator for COM.
Consistency. If one wants to test internal details - what is going to be our guideline to decide when to use reflection vs. introducing internal contracts? We use reflection everywhere, except a few places. |
|
For me, the reason that these contracts are different than our usual internal contracts is that they are |
I think that is merely because using Reflection was deemed annoying at the time. It can't be used as the singular justification because marking them |
I am not sure what are the other internal contracts you have in mind. For example, we have number of internal methods that support debugger, they are tested via reflection, and that form the contract between the runtime and the debugger (this contract is public from CoreLib). If you flip these methods touched in this PR to internal, I believe that the product is still going to work fine, just the tests are going to break. (ICastable is an exception that will go away.) |
|
Does the hosting API surface (coreclr_get_delegate) support getting |
I think so. I have just checked - I do not see any visibility checks. |
|
I'll go down the route of converting our tests to use reflection. For ICastable, I've got an idea to avoid ref-emit and I'll see how people feel about it. Since ICastable will go likely go away in the .NET 7 timeframe anyway, we have some more flexibility in what we do in the interim. |
…posed corelib types. Use reflection for the hosting tests as they'll be around indefinitely. Use the ref-assembly trick for ICastable testing since ICastable testing would require Ref-emit which makes the test significantly less readable and since ICastable will be going away within the .NET 7 timeframe. Supercedes dotnet#61754
|
Closing in favor of #61802 |
ghost
locked as resolved and limited conversation to collaborators
Introduce non-shipped System.Runtime.Internal assembly for internal test usage only and remove support from our test system. Any internal APIs (primarily in the hosting space) that are needed for testing that we don't want to expose publicly should be exposed via System.Runtime.Internal for our testing infrastructure to reference.
This allows us to simplify our testing infrastructure to not have to handle tests that reference only CoreLib and to not have to build CoreLib in our test build job.