Fix locating files in GetCommonSourceFiles#125123
Fix locating files in GetCommonSourceFiles#125123mrvoorhe wants to merge 1 commit intodotnet:mainfrom
GetCommonSourceFiles#125123Conversation
`_testCase.RootCasesDirectory` is meant to be configurable. For example, Unity uses the same test framework to run tests that live in `Unity.Linker.Tests.Cases`. When we run tests, `_testCase.RootCasesDirectory` is going to be the path to our `Unity.Linker.Tests.Cases` directory. When that happens `GetCommonSourceFiles` cannot locate the files relative to `_testCase.RootCasesDirectory`. Since `GetCommonSourceFiles` is trying to explicitly locate files relative to the ILLink test case directory structure the code should be using a known directory to start from rather than a path that is configurable. I added the new `GetMonoLinkerTestsDirectory` directory to `TestCaseCompilationMetadataProvider.cs` instead of adding it to `PathUtilities` because it makes life a little easier. Unity has to provide it's own copy of `PathUtilities` in order to add the `bin` directory in the path returned by `GetTestAssemblyRoot`. Which means having this path location in `PathUtilities` leads to more wire up work for us. That said, if you'd prefer `GetMonoLinkerTestsDirectory` be in `PathUtilities` I can move it there and deal with the extra wiring on our end. While I was here I thought I'd make `GetCommonSourceFiles` virtual just in case Unity needs to override it in the future.
|
@sbomer please take a look |
github-actions
bot
added
the
area-Tools-ILLink
dotnet-policy-service
bot
added
linkable-framework
|
Tagging subscribers to this area: @agocke, @dotnet/illink |
There was a problem hiding this comment.
Pull request overview
This PR updates the ILLink test framework’s GetCommonSourceFiles path resolution so it doesn’t depend on the configurable _testCase.RootCasesDirectory, which can point at non-ILLink test case trees (e.g., Unity’s).
Changes:
- Switch
GetCommonSourceFilesto compute paths from a “known” test framework directory rather than_testCase.RootCasesDirectory. - Add
GetMonoLinkerTestsDirectoryhelper (viaCallerFilePath) to find theMono.Linker.Testsdirectory. - Make
GetCommonSourceFilesvirtual to allow downstream override.
|
|
||
| // Currently this isn't needed anywhere else. Putting it in PathUtilities would require Unity to do some extra wiring up work in order | ||
| // to get this working. It seems simpler to just leave this here. | ||
| static NPath GetMonoLinkerTestsDirectory([CallerFilePath] string thisFile = null) => thisFile.ToNPath().Parent.Parent; |
There was a problem hiding this comment.
GetMonoLinkerTestsDirectory() relies on [CallerFilePath], which bakes the build-time source path into the assembly. This bypasses the existing runtime-configured test root (PathUtilities.GetTestsSourceRootDirectory via AppContext) and can break when the binaries are executed from a different checkout/path than they were built in. Consider resolving these paths from PathUtilities.GetTestsSourceRootDirectory() (the known root used elsewhere in the test framework) instead of CallerFilePath, and drop GetMonoLinkerTestsDirectory entirely (or implement it in terms of PathUtilities).
| static NPath GetMonoLinkerTestsDirectory([CallerFilePath] string thisFile = null) => thisFile.ToNPath().Parent.Parent; | |
| static NPath GetMonoLinkerTestsDirectory() => PathUtilities.GetTestsSourceRootDirectory(); |
| public virtual IEnumerable<NPath> GetCommonSourceFiles() | ||
| { | ||
| yield return _testCase.RootCasesDirectory.Parent | ||
| yield return GetMonoLinkerTestsDirectory().Parent | ||
| .Combine("Mono.Linker.Tests.Cases.Expectations") | ||
| .Combine("Support") | ||
| .Combine("DynamicallyAccessedMembersAttribute.cs"); | ||
|
|
||
| var sharedDir = _testCase.RootCasesDirectory.Parent.Parent | ||
| var sharedDir = GetMonoLinkerTestsDirectory().Parent.Parent | ||
| .Combine("src") |
There was a problem hiding this comment.
GetMonoLinkerTestsDirectory() is called twice in this method; consider storing it in a local variable (e.g., testsDir) to avoid repeated path conversion and to make the subsequent path construction easier to read and maintain.
|
I'm looking into the failures |
_testCase.RootCasesDirectoryis meant to be configurable. For example, Unity uses the same test framework to run tests that live inUnity.Linker.Tests.Cases. When we run tests,_testCase.RootCasesDirectoryis going to be the path to ourUnity.Linker.Tests.Casesdirectory. When that happensGetCommonSourceFilescannot locate the files relative to_testCase.RootCasesDirectory.Since
GetCommonSourceFilesis trying to explicitly locate files relative to the ILLink test case directory structure the code should be using a known directory to start from rather than a path that is configurable.I added the new
GetMonoLinkerTestsDirectorydirectory toTestCaseCompilationMetadataProvider.csinstead of adding it toPathUtilitiesbecause it makes life a little easier. Unity has to provide it's own copy ofPathUtilitiesin order to add thebindirectory in the path returned byGetTestAssemblyRoot. Which means having this path location inPathUtilitiesleads to more wire up work for us. That said, if you'd preferGetMonoLinkerTestsDirectorybe inPathUtilitiesI can move it there and deal with the extra wiring on our end.While I was here I thought I'd make
GetCommonSourceFilesvirtual just in case Unity needs to override it in the future.